[WIP] Philip playground by pschlump · Pull Request #26 · threshold-network/keep-core

pschlump · 2018-02-02T14:56:09Z

Preliminary contract for support of

verifying a user is staked - just returns true
requesting a random number generation
completion of random number generation

All very preliminary - and probably missing parameters - missing validation code etc...

Preliminary code in interface/m4 for testing of Go calls to this contract and event response.

Shadowfiend · 2018-02-02T15:06:46Z

!! This looks fun. I'll try to run it a little later :)

Thoughts on what we want to do regarding checking in generated code vs generating it as part of the build?

mhluongo · 2018-02-03T16:49:20Z

Am I the only one who wants to use the Docker build in place of eg make? It's already standardized and having two build systems seems strange.

pschlump · 2018-02-03T22:12:14Z

I have used both. IMHO it is much harder to get docker do do simple tasks. Before we are done we should create a Docker base build because it captures all the dependencies and is fully reproducible.

…

On Sat, Feb 3, 2018 at 9:49 AM, Matt Luongo ***@***.***> wrote: Am I the only one who wants to use the Docker build in place of eg make? It's already standardized and having two build systems seems strange. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#26 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAhMQQCs2B-kntsSgQr6UZ71xhKFh8-qks5tRI4QgaJpZM4R3TEA> .

-- Philip Schlump

mhluongo · 2018-02-03T23:35:37Z

Agreed that make is simpler, but we already have a Dockerfile and are using it to do CI and (eventually) reproducible builds- why would we maintain two? It'd lead to some people updating the Makefile and others updating the Dockerfile, I expect.

mhluongo · 2018-02-04T00:30:04Z

This came up in #28, but I realized it's an issue across all of us- we need to use the same precise language discussing this stuff or we'll keep talking past each other. Eg "relay request" instead of "requesting random number generation", etc. Details in the glossary (pinned https://github.com/keep-network/keep-core/blob/57573951db51082cc743ccca442528004b20466d/docs/glossary.adoc)

Shadowfiend

Left some notes on KStart.sol.

Shadowfiend · 2018-02-05T19:54:14Z

+    /* Inputs: RequestID(user random), payment(eth), blockReward(keep), seed(number) */
+    /* Outputs: RequestID ??? Should this be an output instead of an input? */
+	// This would be better if the "RequestID" was a generated sequence number and returnd. -- How do I do that?
+    function requestRandomNumber(uint256 _RequestID, uint256 _payment, uint256 _blockReward, uint256 _seed) public {


RequestID should be computed by the contract.

done

Now generated id in a 2nd contract - so that we hopefully will not update the contract and overwrite our IDs.

Shadowfiend · 2018-02-05T19:56:24Z

+	}
+
+    /* Inputs: RequestID(user random), payment(eth), blockReward(keep), seed(number) */
+    /* Outputs: RequestID ??? Should this be an output instead of an input? */


As we start polishing this, let's use NatSpec.

Shadowfiend · 2018-02-05T19:57:21Z

+	// Missing some sort of failed-validation, or group broke call?
+
+	// Function that can be called to get your random number
+	function getRandomNumber() view public returns ( uint status, uint256 theRandomNumber) {


I don't know that we need a lookup function, vs just relying on the event being emitted?

I think that having this makes testing easier. I don't see the downside in including it.

My reasoning: if we include it we have to explicitly track the random number per id (= more gas?), vs only figuring it out when we need to (specifically, when dealing with validation--this assumes we can figure it out without tracking it explicitly, of course).

The function looks up the random number using the RequestID - and it is just a constant lookup - no Gas required. My testing has changed a bunch too.

This function may be removed in a day or two.

Shadowfiend · 2018-02-05T19:57:42Z

+    mapping (address => uint256) public number;
+
+    /* This generates a public event on the blockchain that will notify clients */
+    event RelayRequestReady(uint256 RequestID, uint256 payment, uint256 blockReward, uint256 seed);


Let's do this more in the form RelayEntryRequested.

Shadowfiend · 2018-02-05T19:58:04Z

+    mapping (address => uint256) public blockReward;
+    mapping (address => uint256) public seed;
+    mapping (address => uint) public complete;
+    mapping (address => uint256) public number;


These mappings go from address, but I think they should mostly start from uint256, which is a requestID?

That is, we should probably map a requestID to a payment, and a requestID to a block reward.

What can we use complete for? I also don't think we really need number (see below) or seed (that'll be in the outgoing event).

I am removing complete - a misunderstanding from the california work. Changing to map from requestID to these values.

Shadowfiend · 2018-02-05T20:35:55Z

@@ -0,0 +1,67 @@
+pragma solidity ^0.4.19;
+
+contract KStart { 


I think this can simply be KeepRelayBeacon for now, maybe?

Let's also make sure to phrase everything here in terms of relay entries, rather than random numbers.

Shadowfiend · 2018-02-05T20:51:30Z

+
+    /* This generates a public event on the blockchain that will notify clients */
+    event RelayRequestReady(uint256 RequestID, uint256 payment, uint256 blockReward, uint256 seed);
+    event RandomNumberReady(uint256 RequestID, bool Valid, uint256 RandomNumber);


Similarly, RelayEntryGenerated.

Shadowfiend · 2018-02-05T20:52:07Z

+    }
+
+	// threshold relay has a number, or failed and passes _Valid as false
+    function randomNumberComplete(uint256 _RequestID, bool _Valid, uint256 _theRandomNumber) public {


I don't think the relay entry publish call would accept a _Valid boolean; rather that, will be a separate call from the validation nodes to mark something as invalid (and trigger the relevant cascading actions).

Shadowfiend · 2018-02-05T21:11:18Z

Am I the only one who wants to use the Docker build in place of eg make? It's already standardized and having two build systems seems strange.

I don't consider docker build a build system for our application, I consider it the bit that sets up our docker image. The Dockerfile should just run make IMO.

I see make as answering “how do we build our project”.

I see docker as answering one variation of “how do we distribute our project”.

pschlump · 2018-02-05T21:11:19Z

+	}
+
+    /* This unnamed function is called whenever someone tries to send ether to it */
+    function () public {


Is this needed - do we care if people can send ETH to the contract?

I think we do, yes.

We need a way to get them their money back, yeah

Adding this prevents them sending in the first place. Is that OK?

Yes. Unless we specifically intend on receiving that ETH and doing something with it, which we currently don't. If we do, we can adjust the function.

mhluongo · 2018-02-06T03:00:57Z

I don't consider docker build a build system for our application, I consider it the bit that sets up our docker image. The Dockerfile should just run make IMO.

I see make as answering “how do we build our project”.

I see docker as answering one variation of “how do we distribute our project”.

... I get the appeal, I really do. But I've seen this show before. The Dockerfile needs to understand as much as a Makefile to properly cache, and we already have all make-like functionality we need in CircleCI. Using both will lead to a dual build system.

🙏 spend a little time cozying up with Docker and CircleCI before we pull the trigger on adding make to the mix.

Shadowfiend · 2018-02-06T12:36:45Z

to properly cache

Can you explain some more about this/point to relevant docs? Not sure I follow.

Shadowfiend · 2018-02-06T13:59:49Z

Going to pull the Makefile discussion into an issue; let's try and resolve it this week, but decide it independently of this PR.

mhluongo · 2018-02-06T14:00:49Z

Docker builds cache images statement by statement in the Dockerfile, so the order and granularity of build steps is important. Eg

RUN go build
RUN apt-get install X

Isn't a good idea, because whenever the Go code changes, apt-get is forced to re-run.

… pass

Fix ongoing rewards miscalculation

pschlump added 11 commits January 29, 2018 10:34

Test of branch

b674b62

Adding .gitignore for temporary files starting with ,

57f1ed7

Adding .gitignore for temporary files starting with ,

3056d7d

Checkin of testing contract

611bfb5

Adding test go interface code

c2634a5

Compiled and tested with ref to 'name' in contract

5b43074

Compiled and tested with ref to 'name' in contract

9816eac

Compiled and tested with ref to 'name' in contract

47bd6e4

Compiled and tested with ref to 'name' in contract

9f6fc00

Preliminary contract with some calls completed.

91d376e

Very prelim version of watch on event

f33aaac

Adding non-abigen method for watching events

2ba9d0d

Shadowfiend reviewed Feb 5, 2018

View reviewed changes

pschlump commented Feb 5, 2018

View reviewed changes

Shadowfiend mentioned this pull request Feb 6, 2018

To Makefile or not to Makefile, that is the question #30

Closed

pschlump added 5 commits February 7, 2018 13:05

Adding ID generation contract for RequestIDs

1f9ed01

Adding ID generation contract for RequestIDs

b8ad9d4

Adding prelim. vesion of some tools for dealing with contract addresses

53fc246

Added in params for other calls to contract

26e120b

Params fixed from Atlanta Meeting

7280fe8

pschlump added 26 commits February 21, 2018 11:30

A test with a PASS

b6949c4

Update

d779b3e

WIP

c0936d1

Fixed a number of small problems in test

16d275c

Implemented last 2 events, added documentation.

67ba9a5

Fixed line lengths to less than 120.

cf96e45

Fixed line lengths to less than 120.

e3241a8

Fixed line lengths to less than 120.

2fd384a

Fixed line lengths to less than 120.

15a17c0

Merge branch 'master' into philip-playground

49d9e0b

Fixed spelling of Staked

9f67ab1

Added a note that you have to compile Geth from source

7c4018b

Fixed keep.network to keep-network so go dep works (and code compiles)

4da31e0

Fixed contract to accept payments for *signatures*

a4530a8

Fixed contract to accept payments for *signatures*

222614b

Now supprts correct way to send ETH to contract

0e85db4

Now supprts correct way to send ETH to contract

983df3f

Notes from meeting

d1be4c5

Added a main program with some CLI params

403260c

Build out more on example main()

07d2d97

Layout of code

8a3efb0

Watch the blockchain for a specified block and waith for that time to…

49cf841

… pass

Read in config file and set defaults

e9dc9d2

A common config, BeaconConfig, and necessary chagnes

f37c1b4

A common config, BeaconConfig, and necessary chagnes

84a0c99

WIP - change to note

0066a1a

pschlump closed this Mar 29, 2018

rargulati mentioned this pull request Apr 24, 2018

Introduce basic network Identity package #82

Closed

2 tasks

pschlump deleted the philip-playground branch May 25, 2018 13:35

dimpar pushed a commit that referenced this pull request Feb 10, 2023

Merge pull request #26 from manumonti/fix-ongoing-rewards

c50fa78

Fix ongoing rewards miscalculation

Uh oh!

Conversation

pschlump commented Feb 2, 2018

Uh oh!

Shadowfiend commented Feb 2, 2018

Uh oh!

mhluongo commented Feb 3, 2018

Uh oh!

pschlump commented Feb 3, 2018 via email

Uh oh!

mhluongo commented Feb 3, 2018

Uh oh!

mhluongo commented Feb 4, 2018

Uh oh!

Shadowfiend left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pschlump Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shadowfiend commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhluongo commented Feb 6, 2018

Uh oh!

Shadowfiend commented Feb 6, 2018

Uh oh!

Shadowfiend commented Feb 6, 2018

Uh oh!

mhluongo commented Feb 6, 2018

Uh oh!

Reviewers

Assignees

Labels

pschlump Feb 5, 2018 •

edited

Loading