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: properly set callvalue in create rollup #26

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Conversation

spsjvc
Copy link
Member

@spsjvc spsjvc commented Dec 20, 2023

The function createRollupGetCallValue was being called without applying defaults, which means params.deployFactoriesToL2 was set to false (even though are defaults set it to true) so the call value was always 0.

Now we properly apply defaults to input params first.

I also added one extra assertion to make sure everything was executed properly and the logs can be successfully decoded.

The test failing in CI should be ignored as it's not set up properly, and should be tried out locally.

@spsjvc spsjvc changed the title test: assert is able to decode deployed core contracts fix: properly set callvalue in create rollup Dec 21, 2023
@spsjvc spsjvc changed the base branch from main to ci-nitro-testnode-flags December 21, 2023 11:17
@spsjvc spsjvc force-pushed the test-add-assertion branch 2 times, most recently from 514d388 to 4fe0adb Compare December 21, 2023 15:04
@spsjvc spsjvc changed the base branch from ci-nitro-testnode-flags to main December 21, 2023 15:25
@spsjvc spsjvc marked this pull request as ready for review December 21, 2023 15:29
Copy link
Contributor

@TucksonDev TucksonDev left a comment

Choose a reason for hiding this comment

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

LGTM. CI is failing, but it was tested with a custom script.

@spsjvc spsjvc merged commit 764c9c1 into main Dec 21, 2023
5 of 6 checks passed
@spsjvc spsjvc deleted the test-add-assertion branch December 21, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants