Skip to content
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

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

dbeal-eth
Copy link
Contributor

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.

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.
@dbeal-eth dbeal-eth added the bug Something isn't working label Feb 12, 2025
@dbeal-eth dbeal-eth self-assigned this Feb 12, 2025
@@ -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"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was testing something; probably should be reverted back though

Comment on lines +111 to +115
new ChainDefinition(deployInfo.def, false, {
chainId,
timestamp: deployInfo.timestamp || 0,
package: { version: '0.0.0' },
}),
Copy link
Member

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?

Copy link
Member

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"]
Copy link
Member

@mjlescano mjlescano Feb 12, 2025

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] %>?

@mjlescano
Copy link
Member

Something appears to be fishy? socket reports a lot of dependencies changes here: #1691 (comment), but there are no changes on any package.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants