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.

Sign up for GitHub

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

Jump to bottom

feat(executor): remove modexp limit #1119

Merged
merged 3 commits into from
Apr 3, 2023
Merged

feat(executor): remove modexp limit #1119

merged 3 commits into from
Apr 3, 2023

Conversation

driftluo
Copy link
Contributor

@driftluo driftluo commented Mar 30, 2023 •
edited by KaoImin
Loading

What this PR does / why we need it:

This PR:

  1. remove modexp's limit of number size
  2. remove modexp's length check

Special notes for your reviewer:

NIL

CI Description

CI Name Description
Chaos CI Test the liveness and robustness of Axon under terrible network condition
Cargo Clippy Run cargo clippy --all --all-targets --all-features
Coverage Test Get the unit test coverage report
E2E Test Run end-to-end test to check interfaces
Code Format Run cargo +nightly fmt --all -- --check and cargo sort -gwc
Web3 Compatible Test Test the Web3 compatibility of Axon
v3 Core Test Run the compatibility tests provided by Uniswap V3
OCT 1-5 | 6-10 | 11 | 12-15 | 16-19 Run the compatibility tests provided by OpenZeppelin

CI Usage

Check the CI you want to run below, and then comment /run-ci.

CI Switch

  • Chaos CI
  • Cargo Clippy
  • Coverage Test
  • E2E Tests
  • Code Format
  • Unit Tests
  • Web3 Compatible Tests
  • OCT 1-5 And 12-15
  • OCT 6-10
  • OCT 11
  • OCT 16-19
  • v3 Core Tests

@driftluo driftluo requested a review from a team as a code owner March 30, 2023 08:13
@driftluo driftluo requested review from peterzhb and ahonn March 30, 2023 08:13
@driftluo
Copy link
Contributor Author

/run-ci

@axon-bot
Copy link

axon-bot bot commented Mar 30, 2023

CI tests run on commit:

CI test list:

Please check ci test results later.

ahonn
ahonn previously approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@ahonn ahonn left a comment

Choose a reason for hiding this comment

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

LGTM

@KaoImin KaoImin changed the title feat: remove modexp limit feat(executor): remove modexp limit Mar 30, 2023
@driftluo driftluo force-pushed the remove-modexp-limit branch 4 times, most recently from cce4df0 to c77c927 Compare March 31, 2023 04:51
blckngm
blckngm reviewed Mar 31, 2023
View reviewed changes
core/executor/src/precompiles/modexp.rs
start = len;
}

let mut end = start.wrapping_add(size);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this actually wraps, won't data[start..end] panic? How about using saturating_add instead.

@blckngm
Copy link
Contributor

blckngm commented Mar 31, 2023

It might still be better to calculate gas cost without allocating for full b, e, m values. (You only need at most 32 bytes of e.)

@KaoImin
Copy link
Contributor

KaoImin commented Apr 3, 2023

/run-ci

@axon-bot
Copy link

axon-bot bot commented Apr 3, 2023

CI tests run on commit:

CI test list:

  • Code Format
  • Cargo Clippy
  • Unit Tests
  • Web3 Compatible Tests

Please check ci test results later.

KaoImin
KaoImin previously approved these changes Apr 3, 2023
View reviewed changes
@KaoImin
Copy link
Contributor

KaoImin commented Apr 3, 2023

/run-ci

@axon-bot
Copy link

axon-bot bot commented Apr 3, 2023

CI tests run on commit:

CI test list:

  • Code Format
  • Cargo Clippy
  • Unit Tests
  • Web3 Compatible Tests

Please check ci test results later.

@KaoImin KaoImin added this pull request to the merge queue Apr 3, 2023
Merged via the queue into main with commit 73b6208 Apr 3, 2023
@KaoImin KaoImin deleted the remove-modexp-limit branch April 3, 2023 11:49
@wenyuanhust wenyuanhust mentioned this pull request Apr 8, 2023
Closed
hongda3141 pushed a commit to hongda3141/axon that referenced this pull request Apr 10, 2023
@dependabot
feat: eip-155
3941e9c 
parent 98a8216
author michael <michael@michael> 1679672226 +0800
committer michael <michael@michael> 1681098977 +0800

parent 98a8216
author michael <michael@michael> 1679672226 +0800
committer michael <michael@michael> 1681098964 +0800

feat(eip-155): init

add logic and e2e test case

fix: handle panic and add one unit test

style: optimize redundancy

test: add rawtx test

style: cargo fmt

fix: change u64 to option of chain_id

style: lint

test: fix test result

test: add EIP-155 and preEIP-155 tests

test: add pre-eip-155 legacy encode test case

test: add legacy with chain_id test case

test: add eip1820 test cases

chore(deps): bump serde from 1.0.157 to 1.0.159 (axonweb3#1114)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

refactor: change the bound of ExecutorAdapter trait (axonweb3#1112)

chore(deps): bump proc-macro2 from 1.0.52 to 1.0.55 (axonweb3#1132)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

feat(executor): remove modexp limit (axonweb3#1119)

refactor: change the maximum RPC gas cap limit (axonweb3#1133)

chore(deps): bump peter-evans/create-pull-request from 4 to 5 (axonweb3#1141)

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@Flouse Flouse added t:feature and removed feature labels Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blckngm blckngm blckngm left review comments

@KaoImin KaoImin KaoImin approved these changes

@ahonn ahonn ahonn left review comments

@peterzhb peterzhb Awaiting requested review from peterzhb peterzhb is a code owner automatically assigned from axonweb3/axon-dev

Assignees
No one assigned
Labels
t:feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@driftluo @blckngm @KaoImin @ahonn @Flouse