Skip to content

Conversation

@ermalkaleci
Copy link
Contributor

@ermalkaleci ermalkaleci commented Dec 16, 2021

closes #1702

@ermalkaleci ermalkaleci requested a review from xlc December 16, 2021 20:12
@xlc xlc requested a review from zjb0807 December 22, 2021 22:22
Copy link
Contributor

@zjb0807 zjb0807 left a comment

Choose a reason for hiding this comment

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

Please merge master. AcalaMultiSignature::Eip1559 was added.

@ermalkaleci ermalkaleci changed the title check evm nonce when converting transaction handle nonce for eth tx Dec 24, 2021
Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

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

I am not sure if this can handle recursive calls. i.e. when an evm call somehow triggers a dispatch of another evm call. this is not yet possible but could be in future.

@ermalkaleci
Copy link
Contributor Author

ermalkaleci commented Dec 24, 2021

I am not sure if this can handle recursive calls. i.e. when an evm call somehow triggers a dispatch of another evm call. this is not yet possible but could be in future.

@xlc maybe we can do take so skip_nonce_incremental is read once

Copy link
Member

@xlc xlc left a comment

Choose a reason for hiding this comment

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

A DispatchError should still not resulting any state change. This means you will want to make it success even if the evm execution failed and emit another event to indicate the error

@zjb0807 zjb0807 requested a review from xlc January 6, 2022 08:20
@zjb0807
Copy link
Contributor

zjb0807 commented Jan 6, 2022

@ermalkaleci I think doing it in post_dispatch is better. Avoid modifying the EVM.

@ermalkaleci
Copy link
Contributor Author

@ermalkaleci I think doing it in post_dispatch is better. Avoid modifying the EVM.

@zjb0807 nonce is incremented before call and after create

@zjb0807
Copy link
Contributor

zjb0807 commented Jan 6, 2022

@zjb0807 nonce is incremented before call and after create

But I think the #[transactional] is more important. The nonce will be increased in the evm module. If the tx returns an error without inc_nonce in evm, the nonce will be increased in post_dispatch.

@zjb0807
Copy link
Contributor

zjb0807 commented Jan 6, 2022

ensure!(
Pallet::<T>::can_call_contract(&target, &source),
Error::<T>::NoPermission
);

Such as this error, we also need to increase nonce.

@zjb0807
Copy link
Contributor

zjb0807 commented Jan 6, 2022

self.state.inc_nonce(caller);

self.state.inc_nonce(caller);

Or remove these inc_nonce in evm module, increase the evm nonce like frame_system::Account nonce in pre_dispatch.

@zjb0807
Copy link
Contributor

zjb0807 commented Jan 7, 2022

https://github.com/ethereum/go-ethereum/blob/12f971fb2d/core/state_transition.go#L321

It seems that there is no need to guarantee that the nonce must be increased. In ethereum, there are some checks before increasing nonce.

@xlc xlc requested a review from shaunxw January 9, 2022 22:12
@zjb0807 zjb0807 marked this pull request as ready for review January 10, 2022 01:27
@zjb0807 zjb0807 requested a review from shaunxw January 10, 2022 01:32
@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #1707 (f9f861e) into master (6c7f819) will increase coverage by 6.16%.
The diff coverage is 77.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1707      +/-   ##
==========================================
+ Coverage   66.91%   73.08%   +6.16%     
==========================================
  Files         224      173      -51     
  Lines       19764    19182     -582     
==========================================
+ Hits        13226    14020     +794     
+ Misses       6538     5162    -1376     
Impacted Files Coverage Δ
ecosystem-modules/compound-cash/src/lib.rs 0.00% <0.00%> (-7.15%) ⬇️
inspect/src/lib.rs 0.00% <ø> (-35.78%) ⬇️
modules/evm-utiltity/macro/src/lib.rs 0.00% <0.00%> (ø)
modules/evm/src/runner/state.rs 53.57% <ø> (ø)
modules/evm/src/runner/storage_meter.rs 97.08% <ø> (+8.99%) ⬆️
modules/evm/src/tests.rs 99.14% <ø> (+0.46%) ⬆️
modules/example/src/lib.rs 74.19% <ø> (-5.12%) ⬇️
modules/example/src/tests.rs 100.00% <ø> (ø)
modules/homa-lite/src/lib.rs 89.73% <ø> (-2.43%) ⬇️
modules/homa-lite/src/mock_no_fees.rs 67.92% <ø> (ø)
... and 284 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 486d5bc...f9f861e. Read the comment docs.

@zjb0807 zjb0807 enabled auto-merge (squash) January 11, 2022 06:02
@zjb0807 zjb0807 merged commit dcb01f4 into master Jan 11, 2022
@zjb0807 zjb0807 deleted the check_evm_nonce branch January 11, 2022 06:03
syan095 pushed a commit that referenced this pull request Jan 13, 2022
* origin/master: (53 commits)
  add new audit report
  Use ExitReason::Revert instead of ExitReason::Error (#1772)
  Claim Account to use Eip-712 (#1755)
  Benchmark evm (#1674)
  support evm create rpc and allow H160 default (#1771)
  Update template files license header. (#1770)
  Fix collect_fee (#1766)
  handle nonce for eth tx (#1707)
  updated to the ORML's test coverage file (#1760)
  rm runtime upgrade (#1757)
  Happy new year 2022. (#1761)
  Excluded some files from test coverage (#1759)
  XCM: add deposit error handler for multi-currency adapter. (#1756)
  update stable asset (#1758)
  Fix test coverage for acala (#1590)
  Fix collect_fee (#1754)
  Update HEADER-GPL3
  Update extrinsic-ordering-check-from-bin.yml (#1752)
  Update HEADER-GPL3
  bump version (#1751)
  ...
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.

Support evm.nonce for CheckNonce

5 participants