-
Notifications
You must be signed in to change notification settings - Fork 7
Fix nonce cheatcode #434
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 nonce cheatcode #434
Conversation
| let evm_value = sp_core::U256::from_little_endian(&input.value().as_le_bytes()); | ||
|
|
||
| mock_handler.fund_pranked_accounts(input.caller()); | ||
|
|
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.
Do you know what is the use case/ unit test for this implementation?
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.
Without this I was seeing a lot of contract duplicates errors, in prank cheatcodes, morpho and aave, the reason being no-one was incrementing the nonce between creations, so you end-ed up with the same address.
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.
cc @alexggh (?)
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.
Indeed there are some issues with this in AAVE, but with the previous implementation nonce did not work correctly as well. Requires investigation
failing tests:
Encountered 2 failing tests in tests/deployments/AaveV3PermissionsTest.t.sol:AaveV3PermissionsTest
[FAIL: Contract creation failed: Module(
ModuleError {
index: 3,
error: [
18,
0,
0,
0,
],
message: Some(
"DuplicateContract",
),
},
)] testCheckPermissions() (gas: 736097582)
[FAIL: Contract creation failed: Module(
ModuleError {
index: 3,
error: [
18,
0,
0,
0,
],
message: Some(
"DuplicateContract",
),
},
)] testCheckPermissionsTreasuryPartner() (gas: 736099253)
Encountered 3 failing tests in tests/protocol/configuration/PoolAddressesProvider.t.sol:PoolAddressesProviderTests
[FAIL: expected an emit, but no logs were emitted afterwards. you might have mismatched events or not enough events were emitted] test_new_PoolAddressesProvider() (gas: 297084128)
[FAIL: expected an emit, but no logs were emitted afterwards. you might have mismatched events or not enough events were emitted] test_reverts_setAddressAsProxy_notAuth() (gas: 374763815)
[FAIL: expected an emit, but no logs were emitted afterwards. you might have mismatched events or not enough events were emitted] test_reverts_setAddress_noAuth() (gas: 466201842)
Encountered 1 failing test in tests/extensions/RevenueSplitter.t.sol:RevenueSplitterTest
[FAIL: Contract call failed: Module(
ModuleError {
index: 3,
error: [
40,
0,
0,
0,
],
message: Some(
"BalanceConversionFailed",
),
},
); counterexample: calldata=0x3df6217100000000000001e3d67fbab6a4109b7f8a55d047bb01656029658af795374031 args=[3037099636964839141363158520536338658961557881809371435319345 [3.037e60]]] test_splitNativeFunds_fuzz_max(uint256) (runs: 1, μ: 5638343, ~: 5638343)
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.
- can you add a sanity check between evm and pallet-revive inside
getNonceCallcheatcode? maybe there's a desync - ive tried just preincrementing nonce manually before the
callandcreate, but settingbump_nonceto false - this so far didn't recreate the failure ~20 runs
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 see that reset nonce is failing in unit tests - checking now
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 see that reset nonce is failing in unit tests - checking now
i probably forgot to check if the account is a contract. because if it is the reset should be to 1 instead of 0.
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.
ie change the resetNonceCall handler in strategy/cheatcodes/mod.rs to:
let nonce = ctx.externalities.execute_with(|| {
if AccountInfo::<Runtime>::is_contract(&H160::from_slice(account.as_slice())) {
1
} else {
0
}
});
ctx.externalities.set_nonce(account, nonce);
```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 did it see here 11d0eae, there are more problems I am implementing EIP-1014 as is not correctly implemented
* Implement pending transactions filter --------- Signed-off-by: Alexandru Cihodaru <[email protected]>
No description provided.