Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Retype Gas limit and nonce to u64 #1409

Merged
merged 20 commits into from
May 16, 2023
Merged

Retype Gas limit and nonce to u64 #1409

merged 20 commits into from
May 16, 2023

Conversation

ChihChengLiang
Copy link
Collaborator

@ChihChengLiang ChihChengLiang commented May 14, 2023 •
edited
Loading

Description

  • Gas limit from Word to u64
  • Nonce from Word to u64

Issue Link

Fix #1377

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Contents

  • change geth_type Account's nonce and gas_limit to eth_core::U64. We can not use simple u64 because we need to serialize nonce and gas_limit to hex value with 0x prefix for Geth RPC to use.
  • simple u64 is used in rest of the places.

Rationale

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-mock Issues related to the mock workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels May 14, 2023
@github-actions github-actions bot added crate-external-tracer Issues related to the external-tracer workspace member and removed crate-external-tracer Issues related to the external-tracer workspace member labels May 15, 2023
@ChihChengLiang ChihChengLiang marked this pull request as ready for review May 16, 2023 04:49
ed255
ed255 reviewed May 16, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

Overall looks good! Also good catch on simplifying how accounts are set in tests!
I have only a small request, which is replacing the usage of U256.low_u64() by U256.as_u64() to have safety checks.

bus-mapping/src/circuit_input_builder/input_state_ref.rs Outdated
@@ -318,7 +318,7 @@ impl<'a> CircuitInputStateRef<'a> {
// Perform the write to the account in the StateDB
if matches!(rw, RW::WRITE) {
match op.field {
AccountField::Nonce => account.nonce = op.value,
AccountField::Nonce => account.nonce = op.value.low_u64(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
AccountField::Nonce => account.nonce = op.value.low_u64(),
AccountField::Nonce => account.nonce = op.value.as_u64(),

With as_u64 we get a panic in case the value doesn't fit in u64. I think this can be useful to avoid silent bugs.

Copy link
Collaborator Author

@ChihChengLiang ChihChengLiang May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good idea. Addressed in 0fc8072

eth-types/src/geth_types.rs Outdated
Comment on lines 178 to 179
nonce: tx.nonce.low_u64().into(),
gas_limit: tx.gas.low_u64().into(),
Copy link
Member

Choose a reason for hiding this comment

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

I think using as_u64 is safer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 0fc8072

ed255
ed255 approved these changes May 16, 2023
View reviewed changes
Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ChihChengLiang ChihChengLiang added this pull request to the merge queue May 16, 2023
Merged via the queue into main with commit d21952d May 16, 2023
@ChihChengLiang ChihChengLiang deleted the gas_limit_nonce_to_u64 branch May 17, 2023 08:12
@ChihChengLiang ChihChengLiang mentioned this pull request Sep 12, 2023
Merged
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ed255 ed255 ed255 approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-eth-types Issues related to the eth-types workspace member crate-mock Issues related to the mock workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

align gas_limits & nonce from word to u64
2 participants
@ChihChengLiang @ed255