Skip to content

Conversation

@jackchuma
Copy link
Contributor

No description provided.

@linear
Copy link

linear bot commented Nov 10, 2025

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Nov 10, 2025

✅ Heimdall Review Status

Requirement Status More Info
Reviews 2/2
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 2
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 2
2
1 if commit is unverified 0
Sum 2

@cb-heimdall
Copy link
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/2
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 2
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 2
2
1 if commit is unverified 0
Sum 2

xenoliss
xenoliss previously approved these changes Nov 10, 2025
ELASTICITY = uint32(vm.envUint("FROM_ELASTICITY"));
NEW_ELASTICITY = uint32(vm.envUint("TO_ELASTICITY"));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Once the state overrides are set (as in my suggestion below), then these pre-checks can be included (from here):

Suggested change
function setUp() external view {
vm.assertEq(ISystemConfig(SYSTEM_CONFIG).eip1559Denominator(), DENOMINATOR, "Denominator mismatch");
vm.assertEq(ISystemConfig(SYSTEM_CONFIG).eip1559Elasticity(), ELASTICITY, "Elasticity mismatch");
vm.assertEq(ISystemConfig(SYSTEM_CONFIG).gasLimit(), GAS_LIMIT, "Gas Limit mismatch");
}

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 actually think the precheck doesn't work even with the overrides set. I noticed our usual gas limit script doesn't include the precheck either. I'm fine excluding this

Copy link
Contributor

Choose a reason for hiding this comment

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

What issue did you encounter with the precheck? It works for me locally with the overrides set.

I don't mind either way, whichever you prefer!

@cb-heimdall cb-heimdall dismissed xenoliss’s stale review November 11, 2025 19:42

Approved review 3445054470 from xenoliss is now dismissed due to new commit. Re-request for approval.

@roger-bai-coinbase
Copy link

LGTM

@jackchuma jackchuma merged commit f39053f into main Nov 12, 2025
5 checks passed
@jackchuma jackchuma deleted the jack/update-gas-params branch November 12, 2025 14:31
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.

6 participants