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.

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: fix evm config init #1484

Merged
merged 1 commit into from
Oct 18, 2023
Merged

fix: fix evm config init #1484

merged 1 commit into from
Oct 18, 2023

Conversation

driftluo
Copy link
Contributor

@driftluo driftluo commented Oct 17, 2023

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
@sunchengzhu
Copy link
Collaborator

sunchengzhu commented Oct 17, 2023

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
Collaborator

sunchengzhu commented Oct 18, 2023

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
@yangby-cryptape
Copy link
Collaborator

yangby-cryptape commented Oct 18, 2023

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
Contributor Author

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
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

#[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 previously approved these changes Oct 18, 2023
@KaoImin

This comment was marked as off-topic.

@github-actions
Copy link

CI tests run on commit:

CI test list:

  • Web3 Compatible Tests

Please check ci test results later.

@KaoImin

This comment was marked as abuse.

@github-actions
Copy link

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.

@github-actions
Copy link

CI tests run on commit:

CI test list:

  • Web3 Compatible Tests

Please check ci test results later.

@sunchengzhu
Copy link
Collaborator

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
19 checks passed
@driftluo driftluo deleted the fix-evm-config branch October 18, 2023 11:50
if latest_hardfork_info & &enable_contract_limit_flag == enable_contract_limit_flag {
let handle = MetadataHandle::new(CURRENT_METADATA_ROOT.with(|r| *r.borrow()));
let config = handle.get_consensus_config().unwrap();
Some(config.max_contract_limit as usize)
} else {
None
Some(0x6000)
Copy link
Contributor

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants