-
Notifications
You must be signed in to change notification settings - Fork 24
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
fix(builder): validation of template accesses which depend on chainId
#1691
base: dev
Are you sure you want to change the base?
Conversation
or other globals this basically turned into a refactor because we have to kind of rethink how we are supplying values to the ChainDefinition. But I think its a move in the right direction. Unfortunately refactors like this usually lead to breakage, so probably best to save this for next release.
@@ -15,7 +15,7 @@ options.salt = "second" | |||
options.msg = "a message from second greeter set" | |||
|
|||
[invoke.do_change_greeting2] | |||
target = ["greeters.Greeter"] | |||
target = ["greeters.Greeterz"] |
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 ok?
new ChainDefinition(deployInfo.def, false, { | ||
chainId, | ||
timestamp: deployInfo.timestamp || 0, | ||
package: { version: '0.0.0' }, | ||
}), |
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.
Having to create this dummy object on several places just to be able to instantiate looks prone to error, maybe a helper function to create the ChainDefinition would be useful?
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.
Oh, I see that those are overrides, but are they necessary? looks like they are only used for giving them to the access recorder, in that case wouldn't be enough to just send the dummy object? Why is there an option to send custom overrides?
@@ -1,4 +1,4 @@ | |||
name = "greeter-foundry" | |||
name = "greeter" | |||
version = "<%= package.version %>" | |||
description = "Simple project to verify the functionality of cannon" | |||
keywords = ["sample", "greeter"] |
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.
Would be good to add an action on this or other of the cannonfiles used on the test with Inifinex's case, was is with something like <%= settings.smth['VALUE_' + chainId] %>
?
Something appears to be fishy? socket reports a lot of dependencies changes here: #1691 (comment), but there are no changes on any |
or other globals
this basically turned into a refactor because we have to kind of rethink how we are supplying values to the ChainDefinition. But I think its a move in the right direction.
Unfortunately refactors like this usually lead to breakage, so probably best to save this for next release.