-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Philip playground #26
Conversation
!! 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? |
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 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
|
Agreed that |
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some notes on KStart.sol
.
go/contract/contracts/KStart.sol
Outdated
/* 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestID
should be computed by the contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Now generated id in a 2nd contract - so that we hopefully will not update the contract and overwrite our IDs.
go/contract/contracts/KStart.sol
Outdated
} | ||
|
||
/* Inputs: RequestID(user random), payment(eth), blockReward(keep), seed(number) */ | ||
/* Outputs: RequestID ??? Should this be an output instead of an input? */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we start polishing this, let's use NatSpec.
go/contract/contracts/KStart.sol
Outdated
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that we need a lookup function, vs just relying on the event being emitted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that having this makes testing easier. I don't see the downside in including it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function may be removed in a day or two.
go/contract/contracts/KStart.sol
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this more in the form RelayEntryRequested
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed.
go/contract/contracts/KStart.sol
Outdated
mapping (address => uint256) public blockReward; | ||
mapping (address => uint256) public seed; | ||
mapping (address => uint) public complete; | ||
mapping (address => uint256) public number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am removing complete - a misunderstanding from the california work. Changing to map from requestID to these values.
go/contract/contracts/KStart.sol
Outdated
@@ -0,0 +1,67 @@ | |||
pragma solidity ^0.4.19; | |||
|
|||
contract KStart { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can simply be KeepRelayBeacon
for now, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also make sure to phrase everything here in terms of relay entries, rather than random numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
go/contract/contracts/KStart.sol
Outdated
|
||
/* 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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, RelayEntryGenerated
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
go/contract/contracts/KStart.sol
Outdated
} | ||
|
||
// threshold relay has a number, or failed and passes _Valid as false | ||
function randomNumberComplete(uint256 _RequestID, bool _Valid, uint256 _theRandomNumber) public { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chaged.
I don't consider docker build a build system for our application, I consider it the bit that sets up our docker image. The I see I see docker as answering one variation of “how do we distribute our project”. |
go/contract/contracts/KStart.sol
Outdated
} | ||
|
||
/* This unnamed function is called whenever someone tries to send ether to it */ | ||
function () public { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed - do we care if people can send ETH to the contract?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to get them their money back, yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this prevents them sending in the first place. Is that OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
... 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 🙏 spend a little time cozying up with Docker and CircleCI before we pull the trigger on adding |
Can you explain some more about this/point to relevant docs? Not sure I follow. |
Going to pull the Makefile discussion into an issue; let's try and resolve it this week, but decide it independently of this PR. |
Docker builds cache images statement by statement in the Dockerfile, so the order and granularity of build steps is important. Eg
Isn't a good idea, because whenever the Go code changes, |
Fix ongoing rewards miscalculation
Preliminary contract for support of
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.