migrate contract e2e#2716
Conversation
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: open-junius has repo write permission, >1 year old account, substantial prior Subtensor activity, no Gittensor allowlist hit found; branch migrate-contract-e2e -> devnet-ready. Static review only. This PR does not modify FindingsNo findings. ConclusionNo malicious behavior or security vulnerability was found in the current diff. 🔍 AI Review — Auditor (domain review)VERDICT: 👎 LIKELY Gittensor / established contributor: open-junius is not in the trusted allowlists, but has repo write permission and substantial recent Subtensor PR activity; review calibrated toward preserving migrated E2E coverage. The Auditor proposed a replacement PR description, but the current body is non-trivial; not overwriting. Maintainers: ask the Auditor to regenerate if you want it. Static review only. I did not run the Moonwall suite because the blocking issues are coverage regressions visible directly in the diff. No Duplicate-work check: #2718 ( Findings
Prior-comment reconciliation
ConclusionBlocking because the migration still deletes legacy crowdloan and leasing precompile lifecycle E2E coverage without restoring equivalent Moonwall tests. The new wrapper tests cover useful smoke paths, but they stop before the state transitions and balance/ownership assertions that the deleted suites protected. 📜 Previous run (superseded)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| ] | ||
| }, { | ||
| "name": "zombienet_evm", |
There was a problem hiding this comment.
[MEDIUM] Wire the new EVM suite into CI
This registers a new zombienet_evm Moonwall suite, but .github/workflows/typescript-e2e.yml still only runs dev, zombienet_shield, zombienet_staking, zombienet_coldkey_swap, and zombienet_subnets. As written, the migrated EVM test can go stale because PR CI never invokes pnpm moonwall test zombienet_evm.
Add zombienet_evm to the run-e2e-tests matrix, likely against the release binary unless there is a specific fast-runtime reason.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| allowBuilds: | ||
| '@biomejs/biome': set this to true or false | ||
| '@parcel/watcher': set this to true or false | ||
| cpu-features: set this to true or false | ||
| esbuild: set this to true or false | ||
| msgpackr-extract: set this to true or false | ||
| protobufjs: set this to true or false | ||
| sqlite3: set this to true or false | ||
| ssh2: set this to true or false |
There was a problem hiding this comment.
[LOW] Resolve allowBuilds placeholders
This allowBuilds block still contains generated placeholder values instead of concrete booleans. Since the same packages are already listed under onlyBuiltDependencies, leaving these placeholder strings makes the workspace policy ambiguous and can confuse future pnpm maintenance. Either remove the unfinished block or replace each entry with intentional true/false values.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| allowBuilds: | ||
| '@biomejs/biome': set this to true or false | ||
| '@parcel/watcher': set this to true or false | ||
| cpu-features: set this to true or false | ||
| esbuild: set this to true or false | ||
| msgpackr-extract: set this to true or false | ||
| protobufjs: set this to true or false | ||
| sqlite3: set this to true or false | ||
| ssh2: set this to true or false |
There was a problem hiding this comment.
[LOW] Resolve allowBuilds placeholders
The allowBuilds entries are still literal placeholder strings (set this to true or false). Resolve this before merge by either replacing them with explicit boolean policy values or removing the block if onlyBuiltDependencies above is the intended source of truth. Leaving generated placeholders makes the install policy ambiguous and can confuse future pnpm approve-builds or dependency review work.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
🔄 AI review updated — Skeptic: SAFE |
| Error::<T>::AmountTooLow | ||
| ); | ||
| Self::ensure_stake_operation_limit_not_exceeded(from_hotkey, from_coldkey, netuid)?; | ||
| Self::ensure_available_to_unstake(from_coldkey, netuid, amount)?; |
There was a problem hiding this comment.
[HIGH] Restore sender staking rate-limit validation
transfer_staked_alpha(..., validate_sender: true, ...) is the path pallets/limit-orders uses when collecting staked alpha from sell-order signers, and that call site explicitly relies on validate_sender to check the user's rate limit. The trait contract also says this flag must reject a sender that already staked in the current block. This hunk removes the only sender-side stake-operation-limit check and leaves only ensure_available_to_unstake, so a signer can stake and then transfer that same-block stake into order matching, bypassing the intended anti-same-block staking guard. Restore an equivalent rate-limit check before the availability check; the removed helper name may need to be reintroduced or replaced with the current rate-limit API.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| Error::<T>::AmountTooLow | ||
| ); | ||
| Self::ensure_stake_operation_limit_not_exceeded(from_hotkey, from_coldkey, netuid)?; | ||
| Self::ensure_available_to_unstake(from_coldkey, netuid, amount)?; |
There was a problem hiding this comment.
[HIGH] Restore sender staking rate-limit validation
collect_assets in pallets/limit-orders calls this with validate_sender: true and documents that this path should check the seller's staking rate limit before pulling staked alpha into the pallet intermediary. The current implementation now validates hotkey existence, minimum amount, and locks, but no longer rejects a sender that is still inside the staking operation limit window before debiting stake. That lets repeated sell-side batch executions bypass the economic throttling that ordinary stake movement is supposed to enforce. Restore the sender-side staking rate-limit check before ensure_available_to_unstake.
|
🔄 AI review updated — Skeptic: VULNERABLE |
| '@biomejs/biome': set this to true or false | ||
| '@parcel/watcher': set this to true or false | ||
| cpu-features: set this to true or false | ||
| esbuild: set this to true or false | ||
| msgpackr-extract: set this to true or false | ||
| protobufjs: set this to true or false | ||
| sqlite3: set this to true or false | ||
| ssh2: set this to true or false |
There was a problem hiding this comment.
[LOW] Resolve allowBuilds placeholders
These entries are still the literal placeholder strings from pnpm approve-builds. Please replace each value with an explicit boolean policy, or remove the allowBuilds block if onlyBuiltDependencies is the intended policy source. Leaving placeholders makes the workspace build policy ambiguous and carries forward the prior review finding.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
| '@biomejs/biome': set this to true or false | ||
| '@parcel/watcher': set this to true or false | ||
| cpu-features: set this to true or false | ||
| esbuild: set this to true or false | ||
| msgpackr-extract: set this to true or false | ||
| protobufjs: set this to true or false | ||
| sqlite3: set this to true or false | ||
| ssh2: set this to true or false |
There was a problem hiding this comment.
[LOW] Resolve allowBuilds placeholders
These literal placeholder strings are still committed under allowBuilds. pnpm expects concrete boolean approvals here; leaving set this to true or false makes the build-script policy ambiguous and can break strict install/config validation. Since these packages are already listed in onlyBuiltDependencies, set the corresponding approvals explicitly.
| '@biomejs/biome': set this to true or false | |
| '@parcel/watcher': set this to true or false | |
| cpu-features: set this to true or false | |
| esbuild: set this to true or false | |
| msgpackr-extract: set this to true or false | |
| protobufjs: set this to true or false | |
| sqlite3: set this to true or false | |
| ssh2: set this to true or false | |
| '@biomejs/biome': true | |
| '@parcel/watcher': true | |
| cpu-features: true | |
| esbuild: true | |
| msgpackr-extract: true | |
| protobufjs: true | |
| sqlite3: true | |
| ssh2: true |
| const end = await getCrowdloanEndBlock(); | ||
| const nextIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the full crowdloan precompile lifecycle coverage
This replacement only creates a crowdloan and checks that NextCrowdloanId advances. The deleted contract-tests/test/crowdloan.precompile.test.ts also verified deposit charging, contribute, withdraw, multi-contributor refund after expiry, dissolve returning the creator deposit, and contribute/withdraw against a Substrate-created crowdloan. Those calls are not present anywhere in the new ts-tests/suites/zombienet_evm coverage, so this migration removes the lifecycle and balance invariants the old E2E protected. Please restore equivalent Moonwall coverage before deleting the legacy suite.
| const leasingEndBlock = crowdloanEnd + 200; | ||
| const nextCrowdloanIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createLeaseCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the leasing finalize and termination coverage
The new leasing wrapper test stops after creating the lease crowdloan. The deleted contract-tests/test/leasing.precompile.test.ts contributed the remaining cap, waited for the crowdloan end, finalized it, asserted SubnetLeases, getLease, getLeaseIdForSubnet, beneficiary/contributor shares, then waited for lease expiry, called terminateLease, and checked subnet ownership handoff. None of those finalization, getter, share, expiry, termination, or owner assertions are restored here, leaving the leasing precompile lifecycle untested in the new framework.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| const end = await getCrowdloanEndBlock(); | ||
| const nextIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the full crowdloan precompile lifecycle coverage
This replacement still only verifies createCrowdloan increments NextCrowdloanId; it does not restore the deleted legacy assertions for contribute, withdraw, expired-crowdloan refund, dissolve, creator/contributor balance restoration, or contributing to a crowdloan created through the Substrate pallet. Since this PR removes contract-tests/test/crowdloan.precompile.test.ts, the migration drops coverage for the mutating crowdloan precompile paths that can lock/refund balances. Please add Moonwall coverage for the full lifecycle before deleting the old suite.
| const leasingEndBlock = crowdloanEnd + 200; | ||
| const nextCrowdloanIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createLeaseCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the leasing finalize and termination coverage
The migrated leasing tests create or read a lease crowdloan but never drive it through contribution, crowdloan expiry, finalize, getLease / getLeaseIdForSubnet assertions, contributor share assertions, lease expiry, terminateLease, or the final owner coldkey/hotkey checks. Those were all covered by the deleted contract-tests/test/leasing.precompile.test.ts. Without equivalent Moonwall coverage, this migration stops testing the leasing precompile's state transition and ownership handoff behavior.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
open-junius
requested review from
JohnReedV,
evgeny-s,
gztensor,
l0r1s and
sam0x17
| ], | ||
| "runScripts": [], | ||
| "runScripts": [ | ||
| "generate-types.sh", |
There was a problem hiding this comment.
Do we need those scripts for dev run?
There was a problem hiding this comment.
You are right. dev can run without the papi types. In local, it is ok. But it doesn't work in the CI.
| const end = await getCrowdloanEndBlock(); | ||
| const nextIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the full crowdloan precompile lifecycle coverage
The deleted contract-tests/test/crowdloan.precompile.test.ts did more than create/read smoke coverage: it exercised contribute, withdraw, failed-crowdloan refund, dissolve, balance restoration, and contribution/withdrawal against a Substrate-created crowdloan. The replacement Moonwall coverage only creates a crowdloan and reads getContribution for a prior ID, so regressions in those precompile state transitions and balance effects would no longer be caught. Please restore equivalent Moonwall tests before deleting the legacy suite.
| const leasingEndBlock = crowdloanEnd + 200; | ||
| const nextCrowdloanIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createLeaseCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the leasing finalize and termination coverage
The deleted leasing E2E test covered the full lease lifecycle: create lease crowdloan, contribute the remaining cap, wait for crowdloan end, finalize, assert SubnetLeases, getLease, getLeaseIdForSubnet, contributor shares, wait for lease expiry, terminateLease, and verify lease removal plus owner coldkey/hotkey handoff. The replacement only creates a lease crowdloan and checks that getContributorShare is defined before finalization, so finalize/termination and ownership regressions would escape CI. Please migrate the full lifecycle assertions into this Moonwall suite.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| const end = await getCrowdloanEndBlock(); | ||
| const nextIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the full crowdloan precompile lifecycle coverage
This replacement only asserts that createCrowdloan succeeds and increments NextCrowdloanId. The deleted contract-tests/test/crowdloan.precompile.test.ts suite exercised the actual precompile lifecycle: creator deposit charging, contribute, withdraw, expired refund, dissolve, balance restoration, and contribute/withdraw against a crowdloan created from the Substrate side. Those are the behaviors that catch accounting regressions in the crowdloan precompile; they need equivalent Moonwall coverage before deleting the old suite.
| const leasingEndBlock = crowdloanEnd + 200; | ||
| const nextCrowdloanIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createLeaseCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the leasing finalize and termination coverage
This replacement stops after creating a lease crowdloan. The deleted contract-tests/test/leasing.precompile.test.ts suite contributed the remaining cap, advanced to the crowdloan end, called finalize, asserted the stored lease fields and getLease/getLeaseIdForSubnet/share getters, advanced to lease expiry, called terminateLease, and checked ownership handoff plus lease removal. Without equivalent Moonwall coverage, this migration drops the only E2E protection for the leasing precompile lifecycle.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| const end = await getCrowdloanEndBlock(); | ||
| const nextIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the full crowdloan precompile lifecycle coverage
The deleted contract-tests/test/crowdloan.precompile.test.ts covered create, contribute, withdraw, expiry refund, dissolve, balance restoration, and contribution/withdrawal against a crowdloan created from the Substrate side. The new Moonwall replacement only creates a crowdloan and reads it back, so regressions in the contribution/refund/dissolve lifecycle would no longer be caught. Restore equivalent coverage in this suite before deleting the old contract tests.
| const leasingEndBlock = crowdloanEnd + 200; | ||
| const nextCrowdloanIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createLeaseCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the leasing finalize and termination coverage
The deleted contract-tests/test/leasing.precompile.test.ts finalized a lease crowdloan, asserted lease fields and contributor shares, waited through lease expiry, terminated the lease, and checked subnet ownership handoff. The new Moonwall tests stop at creating/read-smoke paths, so finalize/terminate regressions in the leasing precompile would no longer fail E2E. Restore those lifecycle assertions in the migrated suite.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
| const end = await getCrowdloanEndBlock(); | ||
| const nextIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the full crowdloan precompile lifecycle coverage
This migrated test only creates a crowdloan and waits for NextCrowdloanId to advance. The deleted contract-tests/test/crowdloan.precompile.test.ts suite covered the lifecycle this migration needs to preserve: creator deposit accounting, contribute, withdraw, multi-contributor refund, dissolve, final balance restoration, and contributing/withdrawing against a crowdloan created from the Substrate side. Please port those assertions into the Moonwall suite instead of replacing them with a create/get smoke test.
| const leasingEndBlock = crowdloanEnd + 200; | ||
| const nextCrowdloanIdBefore = await api.query.Crowdloan.NextCrowdloanId.getValue(); | ||
|
|
||
| const createTx = await wrapperContract.createLeaseCrowdloan( |
There was a problem hiding this comment.
[MEDIUM] Migrate the leasing finalize and termination coverage
This migrated leasing test stops after creating a lease crowdloan and checking that the crowdloan id advances. The deleted contract-tests/test/leasing.precompile.test.ts suite exercised the important state transitions: second-wallet contribution to reach the cap, waiting to the crowdloan end, finalizing, checking SubnetLeases, getLease, getLeaseIdForSubnet, contributor share values, waiting to lease expiry, terminating the lease, and verifying subnet owner/hotkey handoff. Please restore equivalent Moonwall coverage so the migration does not silently drop those precompile guarantees.
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👎 |
open-junius
added
the
skip-cargo-audit
3 participants
Description
Migrate contract e2e to new framework.
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.