migrate contract e2e#2716
Conversation
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE BASELINE scrutiny: open-junius has repo write permission and substantial prior subtensor history; branch targets devnet-ready; Gittensor allowlist search had no hit. Static review covered the trusted instructions, cached PR metadata, contributor signals, prior Skeptic output, branch target, changed-file list, and full diff. The PR does not modify FindingsNo findings. Prior-comment reconciliation
ConclusionNo malicious intent or PR-introduced security vulnerability was found in the current diff. The only runtime-file change is a blank-line removal, and the rest of the PR migrates existing EVM transfer coverage into the TypeScript/zombienet test framework. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 LIKELY gittensor; open-junius has write permission and many recent merged subtensor PRs, but is not present in the trusted allowlists. 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 this workspace has no No runtime behavior changed: the only pallet edit is a blank-line removal in Overlapping PRs were checked. #2718 overlaps in contract E2E files but is a broader contracts-E2E optimization branch, while this PR is the focused Moonwall migration for the EVM transfer suite; this PR remains the better candidate for this migration. The other overlaps are incidental workflow, governance, pallet/runtime, or transaction-payment changes rather than duplicate implementations. Findings
Prior-comment reconciliation
ConclusionApproving with one carried-forward low-severity cleanup still outstanding. The EVM transfer E2E coverage is migrated into the TypeScript/Moonwall CI path, and the remaining issue is limited to unresolved pnpm policy placeholders. 📜 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: 👎 |
| 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 new allowBuilds map still contains generated placeholder strings instead of explicit boolean decisions. pnpm 10.26+ defines this field as a map that explicitly allows (true) or disallows (false) dependency build scripts, so leaving set this to true or false here keeps the new policy unresolved and can cause install-time warnings or stricter future failures. (newreleases.io)
| 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 | |
| allowBuilds: | |
| '@biomejs/biome': true | |
| '@parcel/watcher': true | |
| cpu-features: true | |
| esbuild: true | |
| msgpackr-extract: true | |
| protobufjs: true | |
| sqlite3: true | |
| ssh2: true |
|
🔄 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 |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
1 participant
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.