-
Notifications
You must be signed in to change notification settings - Fork 118
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
enforce protocol limits on account updates and events #620
Conversation
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.
LGTM! Feel free to ignore all the nits, but would be nice to add a little test for the filterGroup
logic
Also, re breaking tests, I would suggest adding a mechanism - maybe optional argument to LocalBlockchain
? - to skip the limits check
const proofCost = 10.26; | ||
const signedPairCost = 10.08; | ||
const signedSingleCost = 9.14; | ||
const costLimit = 69.45; |
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 should eventually add this to auto-generation via the snarky_js_constants.ml
mechanism, so it'd guaranteed to stay in sync. It's not urgent though and can be an issue for later
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.
👌🏻
enforces limits on the layout of account updates, sequence events and events, as defined on the protocol side
TODO: fix token tests, since they exceed the newly defined limitscloses #276