Skip to content

test: add Rex4 system contract deployment and idempotence tests#187

Open
viktorcrypt wants to merge 1 commit intomegaeth-labs:mainfrom
viktorcrypt:viktorcrypt/test/rex4-system-contract-deployment
Open

test: add Rex4 system contract deployment and idempotence tests#187
viktorcrypt wants to merge 1 commit intomegaeth-labs:mainfrom
viktorcrypt:viktorcrypt/test/rex4-system-contract-deployment

Conversation

@viktorcrypt
Copy link

Summary

Adds deployment wiring tests for the two Rex4 system contracts:
MegaAccessControl (0x...0004) and MegaLimitControl (0x...0005).

What is tested

  • Activation: verifies both contracts are deployed at the correct
    addresses after apply_pre_execution_changes() on a Rex4 block
  • Idempotence: verifies that calling apply_pre_execution_changes()
    twice does not panic or corrupt state
  • Spec boundary: verifies that on Rex3, neither contract is deployed

Why

The interception behavior of these contracts was already covered,
but the hardfork-gated deployment wiring in block/executor.rs had
no test coverage. These tests close that gap for the Rex4 unstable frontier.

Comment on lines +17 to +18
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

rex4_chain_spec() only activates MegaHardfork::Rex4 without enabling prerequisite hardforks (Rex, Rex1, Rex2, Rex3). In production, all prior hardforks would be active. If apply_pre_execution_changes() deploys earlier system contracts (Oracle, HP Timestamp, Keyless Deploy) conditionally on their hardfork activation, this test won't exercise those paths, potentially masking ordering dependencies.

Consider using MegaHardforkConfig::default().with_all_activated() or chaining all prerequisite hardforks to better reflect production conditions.

Comment on lines +60 to +65
let cache_acc = db.load_cache_account(address).expect("should load cache account");
assert!(
cache_acc.account_info().is_none(),
"{name} should not be deployed for this spec boundary"
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

assert_contract_not_deployed checks cache_acc.account_info().is_none(), which asserts the entire account doesn't exist. This is correct for a fresh MemoryDatabase, but a more precise check would verify the code_hash is KECCAK_EMPTY (no code deployed) rather than asserting the account doesn't exist at all. If the executor ever touches the account for a non-code reason, this assertion would break even though the contract isn't actually deployed.

let db_ref = executor.evm_mut().db_mut();
assert_contract_not_deployed(db_ref, ACCESS_CONTROL_ADDRESS, "MegaAccessControl");
assert_contract_not_deployed(db_ref, LIMIT_CONTROL_ADDRESS, "MegaLimitControl");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Significant setup duplication across all three test functions (~20 lines of identical boilerplate each). The existing test suite typically extracts setup into helper functions. Consider a helper like fn make_executor(spec: MegaSpecId, hardforks: MegaHardforkConfig) -> MegaBlockExecutor<...> to reduce repetition.

@viktorcrypt
Copy link
Author

I addressed the requested changes and force-pushed the updated branch.

The test now uses production-like hardfork activation, checks KECCAK_EMPTY for the non-deployed case, and factors the repeated executor setup into a helper. I also verified it still compiles with cargo +nightly test -p mega-evm --test rex4 --no-run --locked.

Unfortunately GitHub seems to be in a bad state for this PR after yesterday’s repository outage: my original fork is now shown as forked from kinderjoypresents/mega-evm instead of megaeth-labs/mega-evm, and the PR UI is stuck processing updates instead of showing the latest commit.

I’ve created a fresh fork from megaeth-labs/mega-evm too, so if this PR remains broken I can move the branch there and recreate the PR.

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.

2 participants