-
Notifications
You must be signed in to change notification settings - Fork 856
Conversation
c15fd50
to
1496040
Compare
1deed4f
to
f13efaf
Compare
There was a problem hiding this 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.
@@ -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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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
nonce: tx.nonce.low_u64().into(), | ||
gas_limit: tx.gas.low_u64().into(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0fc8072
9f903ec
to
0fc8072
Compare
0fc8072
to
e7430be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Issue Link
Fix #1377
Type of change
Contents
Rationale