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 getSignInput bug for zero maxPriorityFeePerGas #228

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

Conversation

everdimension
Copy link

@everdimension everdimension commented Dec 18, 2024

Explicit zero value should not fallback to maxFeePerGas

See zkSync-Community-Hub/zksync-developers#814

Please run the tests to verify, I haven't been able to run https://github.com/matter-labs/local-setup yet

More details in comment: zkSync-Community-Hub/zksync-developers#814 (comment)

@petarTxFusion
Copy link
Contributor

petarTxFusion commented Dec 18, 2024

Please fix commit message, we follow conventional commit messages, and check errors during build

@everdimension everdimension force-pushed the fix/eip712signer-get-sign-input-bug branch 2 times, most recently from ec2fcf2 to 3952b49 Compare December 18, 2024 12:50
Explicit zero value should not fallback to maxFeePerGas
@everdimension everdimension force-pushed the fix/eip712signer-get-sign-input-bug branch from 3952b49 to 157620b Compare December 18, 2024 12:50
@everdimension
Copy link
Author

@petarTxFusion thx, done!

@everdimension
Copy link
Author

@petarTxFusion It doesn't seem that failed tests are related, but maybe they are, can you check please?

@petarTxFusion
Copy link
Contributor

@petarTxFusion It doesn't seem that failed tests are related, but maybe they are, can you check please?

I will run them on the main branch so we can see

@petarTxFusion
Copy link
Contributor

Tests are passing on the main branch you can see it here. I will have to take a better look at your changes

@everdimension
Copy link
Author

@petarTxFusion Thanks, great! Yes, please do. I added a test that should fail for current implementation
Hopefully this can get merged soon because currently we're blocked by this bug

@everdimension
Copy link
Author

Transaction constructor is also buggy, so: 1e8231e

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