-
Notifications
You must be signed in to change notification settings - Fork 266
feat(entropy): Event V2 spec #2621
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
@@ -83,7 +83,7 @@ impl<T: JsonRpcClient + 'static + Clone> SignablePythContractInner<T> { | |||
{ | |||
// Extract Log from TransactionReceipt. | |||
let l: RawLog = r.logs[0].clone().into(); | |||
if let PythRandomEvents::RequestedFilter(r) = PythRandomEvents::decode_log(&l)? { |
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 naming system seems a bit brittle (I think it's based on the order of the events in the file) but (1) the ABI ordering seems stable and (2) it's temporary anyway so i think it's fine.
* @param provider The address of the registered provider | ||
* @param dummy Additional data field (unused) | ||
*/ | ||
event Registered(address indexed provider, bytes dummy); |
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.
Can we change this parameter name to something like extraArgs
?
Also, I'm pretty sure we will forget to update and remove the (unused)
comment when we start to use this event 😅
req.requester, | ||
req.sequenceNumber, | ||
userRandomNumber, | ||
bytes("") |
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.
shouldn't we include more info like gasLimit from the getgo?
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 have a follow up with this (and actual gas usage) ready to go after this is merged.
* @param sequenceNumber The unique identifier of the request | ||
* @param randomNumber The generated random number | ||
* @param callbackFailed Whether the callback to the caller failed | ||
* @param callbackErrorCode If the callback failed, the error code from the callback. Note |
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.
ErrorCode may not be the best name here since this is the return value of the callback. Also maybe you can specify that this is the first 256 bytes of the return value.
Summary
Add a new set of EntropyEvents to improve on the previous spec. Specifically, add indexing for the salient attributes, stop depending on struct definitions (which, if changed, break the event signature), and add dummy bytes for future extensibility.
In the process, I have omitted events for code paths that we are going to kill off (like the old pull flow).
How has this been tested?