Skip to content

fix: fix evm config init#1484

Merged
KaoImin merged 1 commit intomainfrom
fix-evm-config
Oct 18, 2023
Merged

fix: fix evm config init#1484
KaoImin merged 1 commit intomainfrom
fix-evm-config

Conversation

@driftluo
Copy link
Copy Markdown
Contributor

@driftluo driftluo commented Oct 17, 2023 •
edited by KaoImin
Loading

What this PR does / why we need it?

This PR fixes evm config init with hardfork features

What is the impact of this PR?

No Breaking Change

PR relation:

  • Ref #
CI Settings

CI Usage

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

CI Switch

  • Web3 Compatible Tests
  • OCT 1-5 And 12-15
  • OCT 6-10
  • OCT 11
  • OCT 16-19
  • v3 Core Tests

CI Description

CI Name Description
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

@driftluo driftluo requested a review from KaoImin October 17, 2023 05:39
@driftluo driftluo requested a review from a team as a code owner October 17, 2023 05:39
@driftluo driftluo requested a review from blckngm October 17, 2023 05:39
KaoImin
KaoImin previously approved these changes Oct 17, 2023
View reviewed changes
@sunchengzhu
Copy link
Copy Markdown
Contributor

sunchengzhu commented Oct 17, 2023 •
edited
Loading

This commit (371579d) fixed the issue where max_contract_limit was not taking effect when starting the axon node with hardforks = []. However, the problem still persists when starting with hardforks = ["None"]. Please check. @driftluo
image

@sunchengzhu
Copy link
Copy Markdown
Contributor

sunchengzhu commented Oct 18, 2023 •
edited
Loading

This commit (17c7a66) has been successfully verified locally. @driftluo @KaoImin
Regardless of starting the Axon node with the configuration hardforks = [] or hardforks = ["None"], attempting to deploy a contract that exceeds the max_contract_limit size will result in the following error:

jsonRpcResponse: {
  jsonrpc: '2.0',
  error: {
    code: -49998,
    message: 'CKB-VM error execution reverted: ',
    data: 'CreateContractLimit'
  },
  id: 2
}

sunchengzhu
sunchengzhu previously approved these changes Oct 18, 2023
View reviewed changes
@chaoticlonghair
Copy link
Copy Markdown
Contributor

chaoticlonghair commented Oct 18, 2023 •
edited
Loading 

jsonRpcResponse: {
  jsonrpc: '2.0',
  error: {
    code: -49998,
    message: 'CKB-VM error execution reverted: ',
    data: 'CreateContractLimit'
  },
  id: 2
}

Has nothing to do with the current PR.
Not issue of current PR, it's an already existed issue.

I have already point that in #1457 but be ingored: Why the error message is related to CKB-VM?
I tried some other EVM contracts which must be failed, and they could got the same messages sometimes.

@driftluo
Copy link
Copy Markdown
Contributor Author

driftluo commented Oct 18, 2023

I have already point that in #1457 but be ingored: Why the error message is related to CKB-VM? I tried some other EVM contracts which must be failed, and they could got the same messages sometimes.

https://github.com/axonweb3/axon/blob/main/core/api/src/jsonrpc/error.rs#L57
It should be a display problem, this is the display setting returned by rpc

@KaoImin
Copy link
Copy Markdown
Contributor

KaoImin commented Oct 18, 2023

I have already point that in #1457 but be ingored: Why the error message is related to CKB-VM? I tried some other EVM contracts which must be failed, and they could got the same messages sometimes.

main/core/api/src/jsonrpc/error.rs#L57 It should be a display problem, this is the display setting returned by rpc

axon/core/api/src/jsonrpc/error.rs

Lines 57 to 58 in a2d8e04

#[display(fmt = "CKB-VM error {}", "decode_revert_msg(&_0.ret)")]
VM(TxResp),

The error display should be changed as following:

    #[display(fmt = "EVM error {}", "decode_revert_msg(&_0.ret)")] 
    EVM(TxResp),

KaoImin
KaoImin reviewed Oct 18, 2023
View reviewed changes
Comment thread core/api/src/jsonrpc/error.rs Outdated
KaoImin
KaoImin previously approved these changes Oct 18, 2023
View reviewed changes
@KaoImin

This comment was marked as off-topic.

Sign in to view
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 18, 2023

CI tests run on commit:

CI test list:

  • Web3 Compatible Tests

Please check ci test results later.

@KaoImin

This comment was marked as abuse.

Sign in to view
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 18, 2023

CI tests run on commit:

CI test list:

  • Web3 Compatible Tests

Please check ci test results later.

@KaoImin

This comment was marked as off-topic.

Sign in to view
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 18, 2023

CI tests run on commit:

CI test list:

  • Web3 Compatible Tests

Please check ci test results later.

@KaoImin KaoImin enabled auto-merge October 18, 2023 10:49
@KaoImin KaoImin requested a review from sunchengzhu October 18, 2023 10:55
@sunchengzhu
Copy link
Copy Markdown
Contributor

sunchengzhu commented Oct 18, 2023

The error message returned by commit (fe64e40) is as follows:

jsonRpcResponse: {
  jsonrpc: '2.0',
  error: {
    code: -49998,
    message: 'EVM error execution reverted: ',
    data: 'CreateContractLimit'
  },
  id: 2
}

@KaoImin KaoImin added this pull request to the merge queue Oct 18, 2023
Merged via the queue into main with commit 0ee928d Oct 18, 2023
@driftluo driftluo deleted the fix-evm-config branch October 18, 2023 11:50
Flouse
Flouse reviewed Oct 18, 2023
View reviewed changes
Comment thread core/executor/src/lib.rs
Some(config.max_contract_limit as usize)
} else {
None
Some(0x6000)
Copy link
Copy Markdown
Contributor

@Flouse Flouse Oct 18, 2023

Choose a reason for hiding this comment

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

Is it better to use fn default_max_contract_limit instead of hardcoding 0x6000?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Flouse Flouse Flouse left review comments

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

@chaoticlonghair chaoticlonghair Awaiting requested review from chaoticlonghair

+2 more reviewers

@KaoImin KaoImin KaoImin approved these changes

@sunchengzhu sunchengzhu sunchengzhu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@driftluo @sunchengzhu @chaoticlonghair @KaoImin @Flouse