migrate contract e2e#2716
Conversation
🛡️ AI Review — Skeptic (security review)VERDICT: VULNERABLE BASELINE scrutiny: open-junius has repo write permission, substantial contribution history, no trusted Gittensor allowlist hit, and targets devnet-ready. Reviewed the trusted instructions, cached PR metadata, prior Skeptic comment, contributor signals, branch target, and full static diff. The PR does not modify Findings
Prior-comment reconciliation
ConclusionThe PR is legitimate-looking, but it still removes the sender-side staking rate-limit validation from an on-chain limit-order asset transfer path. That economic guard needs to be restored before merge. 📜 Previous run (superseded)
🔍 AI Review — Auditor (domain review)VERDICT: 👍 LIKELY gittensor; open-junius has write permission and substantial recent subtensor activity, but is not 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. No runtime or pallet code changed, so no spec_version bump is required. The PR wires the new Findings
Prior-comment reconciliation
ConclusionApproving with the prior low-severity cleanup still outstanding. The migrated EVM E2E coverage is wired into CI, 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 |
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.