feat: sui rfq vault and tests#11
Closed
reddyismav wants to merge 3 commits into
Closed
Conversation
|
Important Review skippedToo many files! This PR contains 363 files, which is 63 over the limit of 300. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (363)
You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughNew Sui Move package RFQ Vault implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sui/rfq-vault/Move.toml (1)
1-7: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winPin the Sui CLI/toolchain for
sui/rfq-vault
Move.lockonly fixes framework sources; it does not pin the compiler version. This package depends on implicit framework deps, so older Sui releases can still fail to resolvestd/sui.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sui/rfq-vault/Move.toml` around lines 1 - 7, The RfqVault package is only pinning framework sources indirectly, so the Sui CLI/compiler version can still drift and break resolution of implicit deps like std and sui. Update the Move.toml for RfqVault to explicitly pin the Sui toolchain/compiler version using the package’s standard version pinning mechanism, and keep the existing package/address configuration intact so builds use a consistent Sui release regardless of local CLI version.
🧹 Nitpick comments (2)
sui/rfq-vault/sources/vault.move (2)
209-219: 🔒 Security & Privacy | 🔵 TrivialEmit events for security-sensitive owner operations.
rescuemoves custodied funds out, andset_solver_pubkey/pause/unpause/transfer_ownershipchange the trust and availability posture, yet none emit events. For a fund-holding vault this should emit structured events so off-chain systems can monitor and alert on owner actions (especiallyrescuewithdrawals and solver-key rotations).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sui/rfq-vault/sources/vault.move` around lines 209 - 219, The owner-sensitive Vault operations currently mutate custody and control state without any event emission, so update the relevant entry functions in Vault to publish structured events for monitoring. Add event emission to rescue, set_solver_pubkey, pause, unpause, and transfer_ownership using the existing Vault event patterns/symbols in the module, and include enough fields to identify the action, actor, receiver or new owner, amount, and key change details so off-chain alerting can track withdrawals and governance changes.
132-156: 🔒 Security & Privacy | 🔵 TrivialSolver key is the sole authority over all pooled funds; nonce is signed but unenforced.
Two things to lock down on the settlement signing model (applies to
refundandmark_for_refundtoo):
fulfil/refundtake an arbitrary signedamountandreceiverand withdraw from the per-token pooledBalance. There is no per-quote escrow and no global cap, so a leaked/compromised solver key drains the entire balance for any token type. This is inherent to the design — ensure the solver key uses HSM/threshold custody, support rotation viaset_solver_pubkey, and add off-chain monitoring/alerting onFulfilled/Refundedvolume.
nonceis part of the signedSettlementMessagebut is never checked on-chain; replay protection comes solely fromquote_used. Confirm this is intended and not a missing nonce check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sui/rfq-vault/sources/vault.move` around lines 132 - 156, The settlement flow in fulfil, refund, and mark_for_refund relies on a signed nonce but never enforces it on-chain, so replay protection currently depends only on quote_id usage. Update the Vault settlement path to explicitly validate and record nonce reuse alongside quote_id, or remove nonce from SettlementMessage if it is not meant to be an on-chain guard; make the behavior consistent across fulfil, refund, and mark_for_refund using the existing verify_signature and mark_quote_id flow as the reference points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sui/rfq-vault/tests/vault_tests.move`:
- Around line 52-226: Add negative coverage for the settlement auth paths in the
vault tests by extending the existing settlement helpers and test cases around
fulfil, refund, and mark_for_refund. Create tests that call vault::fulfil,
vault::refund, and vault::mark_for_refund with signatures that are either signed
by the wrong keypair or otherwise malformed, and assert they abort instead of
accepting the request. Reuse the existing helpers like sign_fulfil, sign_refund,
sign_mark_for_refund, and the Vault/shared setup in vault_tests.move so the
failure cases sit alongside the current positive replay/consumption checks.
- Around line 321-355: The admin-path coverage in
owner_can_rescue_and_update_admin_state only checks the happy path for
set_solver_pubkey, pause, unpause, and transfer_ownership, so add negative tests
proving a non-owner is rejected by each of those Vault entrypoints. Use the
existing non_owner_cannot_rescue pattern as a guide, and add separate
expected_failure cases around vault::set_solver_pubkey, vault::pause,
vault::unpause, and vault::transfer_ownership to ensure the owner-only guard in
Vault is enforced consistently.
---
Outside diff comments:
In `@sui/rfq-vault/Move.toml`:
- Around line 1-7: The RfqVault package is only pinning framework sources
indirectly, so the Sui CLI/compiler version can still drift and break resolution
of implicit deps like std and sui. Update the Move.toml for RfqVault to
explicitly pin the Sui toolchain/compiler version using the package’s standard
version pinning mechanism, and keep the existing package/address configuration
intact so builds use a consistent Sui release regardless of local CLI version.
---
Nitpick comments:
In `@sui/rfq-vault/sources/vault.move`:
- Around line 209-219: The owner-sensitive Vault operations currently mutate
custody and control state without any event emission, so update the relevant
entry functions in Vault to publish structured events for monitoring. Add event
emission to rescue, set_solver_pubkey, pause, unpause, and transfer_ownership
using the existing Vault event patterns/symbols in the module, and include
enough fields to identify the action, actor, receiver or new owner, amount, and
key change details so off-chain alerting can track withdrawals and governance
changes.
- Around line 132-156: The settlement flow in fulfil, refund, and
mark_for_refund relies on a signed nonce but never enforces it on-chain, so
replay protection currently depends only on quote_id usage. Update the Vault
settlement path to explicitly validate and record nonce reuse alongside
quote_id, or remove nonce from SettlementMessage if it is not meant to be an
on-chain guard; make the behavior consistent across fulfil, refund, and
mark_for_refund using the existing verify_signature and mark_quote_id flow as
the reference points.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a472848a-c664-4d83-b1a8-39ed798f0a3a
⛔ Files ignored due to path filters (1)
sui/rfq-vault/Move.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
sui/rfq-vault/Move.tomlsui/rfq-vault/sources/vault.movesui/rfq-vault/tests/vault_tests.move
Comment on lines
+52
to
+226
| #[test] | ||
| fun fulfil_with_solver_signature_releases_funds_and_blocks_replay() { | ||
| let mut scenario = setup_funded_vault(2 * AMOUNT); | ||
| scenario.next_tx(OTHER); | ||
| { | ||
| let mut vault = scenario.take_shared<Vault>(); | ||
| let quote_id = quote_id(2); | ||
| let nonce = 7; | ||
| let signature = sign_fulfil<VAULT_TESTS>(&vault, copy quote_id, nonce, AMOUNT, RECEIVER); | ||
|
|
||
| vault::fulfil<VAULT_TESTS>( | ||
| &mut vault, | ||
| copy quote_id, | ||
| nonce, | ||
| AMOUNT, | ||
| RECEIVER, | ||
| signature, | ||
| scenario.ctx(), | ||
| ); | ||
|
|
||
| assert!(vault::vault_balance<VAULT_TESTS>(&vault) == AMOUNT); | ||
| assert!(vault::is_quote_used(&vault, copy quote_id)); | ||
| test_scenario::return_shared(vault); | ||
| }; | ||
| scenario.next_tx(RECEIVER); | ||
| { | ||
| let coin = scenario.take_from_address<Coin<VAULT_TESTS>>(RECEIVER); | ||
| assert!(coin.value() == AMOUNT); | ||
| transfer::public_transfer(coin, OWNER); | ||
| }; | ||
| scenario.end(); | ||
| } | ||
|
|
||
| #[test, expected_failure(abort_code = vault::E_INVALID_QUOTE_ID)] | ||
| fun fulfil_replay_aborts() { | ||
| let mut scenario = setup_funded_vault(2 * AMOUNT); | ||
| scenario.next_tx(OTHER); | ||
| { | ||
| let mut vault = scenario.take_shared<Vault>(); | ||
| let quote_id = quote_id(3); | ||
| let signature = sign_fulfil<VAULT_TESTS>(&vault, copy quote_id, 8, AMOUNT, RECEIVER); | ||
|
|
||
| vault::fulfil<VAULT_TESTS>( | ||
| &mut vault, | ||
| copy quote_id, | ||
| 8, | ||
| AMOUNT, | ||
| RECEIVER, | ||
| copy signature, | ||
| scenario.ctx(), | ||
| ); | ||
| vault::fulfil<VAULT_TESTS>( | ||
| &mut vault, | ||
| quote_id, | ||
| 8, | ||
| AMOUNT, | ||
| RECEIVER, | ||
| signature, | ||
| scenario.ctx(), | ||
| ); | ||
|
|
||
| test_scenario::return_shared(vault); | ||
| }; | ||
| scenario.end(); | ||
| } | ||
|
|
||
| #[test, expected_failure(abort_code = vault::E_INVALID_QUOTE_ID)] | ||
| fun fulfil_after_refund_aborts() { | ||
| let mut scenario = setup_funded_vault(2 * AMOUNT); | ||
| scenario.next_tx(OTHER); | ||
| { | ||
| let mut vault = scenario.take_shared<Vault>(); | ||
| let quote_id = quote_id(4); | ||
| let refund_signature = sign_refund<VAULT_TESTS>(&vault, copy quote_id, 9, AMOUNT, RECEIVER); | ||
| vault::refund<VAULT_TESTS>( | ||
| &mut vault, | ||
| copy quote_id, | ||
| 9, | ||
| AMOUNT, | ||
| RECEIVER, | ||
| refund_signature, | ||
| scenario.ctx(), | ||
| ); | ||
|
|
||
| let fulfil_signature = sign_fulfil<VAULT_TESTS>(&vault, copy quote_id, 10, AMOUNT, RECEIVER); | ||
| vault::fulfil<VAULT_TESTS>( | ||
| &mut vault, | ||
| quote_id, | ||
| 10, | ||
| AMOUNT, | ||
| RECEIVER, | ||
| fulfil_signature, | ||
| scenario.ctx(), | ||
| ); | ||
|
|
||
| test_scenario::return_shared(vault); | ||
| }; | ||
| scenario.end(); | ||
| } | ||
|
|
||
| #[test] | ||
| fun mark_for_refund_consumes_quote_id_and_blocks_refund() { | ||
| let mut scenario = setup_funded_vault(AMOUNT); | ||
| scenario.next_tx(OTHER); | ||
| { | ||
| let mut vault = scenario.take_shared<Vault>(); | ||
| let quote_id = quote_id(5); | ||
| let signature = sign_mark_for_refund(&vault, copy quote_id, 11); | ||
|
|
||
| vault::mark_for_refund(&mut vault, copy quote_id, 11, signature); | ||
|
|
||
| assert!(vault::is_quote_used(&vault, copy quote_id)); | ||
| assert!(vault::is_marked_for_refund(&vault, copy quote_id)); | ||
| test_scenario::return_shared(vault); | ||
| }; | ||
| scenario.end(); | ||
| } | ||
|
|
||
| #[test, expected_failure(abort_code = vault::E_INVALID_QUOTE_ID)] | ||
| fun refund_after_mark_for_refund_aborts() { | ||
| let mut scenario = setup_funded_vault(AMOUNT); | ||
| scenario.next_tx(OTHER); | ||
| { | ||
| let mut vault = scenario.take_shared<Vault>(); | ||
| let quote_id = quote_id(6); | ||
| let mark_signature = sign_mark_for_refund(&vault, copy quote_id, 12); | ||
| vault::mark_for_refund(&mut vault, copy quote_id, 12, mark_signature); | ||
|
|
||
| let refund_signature = sign_refund<VAULT_TESTS>(&vault, copy quote_id, 13, AMOUNT, RECEIVER); | ||
| vault::refund<VAULT_TESTS>( | ||
| &mut vault, | ||
| quote_id, | ||
| 13, | ||
| AMOUNT, | ||
| RECEIVER, | ||
| refund_signature, | ||
| scenario.ctx(), | ||
| ); | ||
|
|
||
| test_scenario::return_shared(vault); | ||
| }; | ||
| scenario.end(); | ||
| } | ||
|
|
||
| #[test] | ||
| fun refund_with_solver_signature_releases_funds() { | ||
| let mut scenario = setup_funded_vault(AMOUNT); | ||
| scenario.next_tx(OTHER); | ||
| { | ||
| let mut vault = scenario.take_shared<Vault>(); | ||
| let quote_id = quote_id(7); | ||
| let signature = sign_refund<VAULT_TESTS>(&vault, copy quote_id, 14, AMOUNT, RECEIVER); | ||
|
|
||
| vault::refund<VAULT_TESTS>( | ||
| &mut vault, | ||
| copy quote_id, | ||
| 14, | ||
| AMOUNT, | ||
| RECEIVER, | ||
| signature, | ||
| scenario.ctx(), | ||
| ); | ||
|
|
||
| assert!(vault::vault_balance<VAULT_TESTS>(&vault) == 0); | ||
| assert!(vault::is_quote_used(&vault, quote_id)); | ||
| test_scenario::return_shared(vault); | ||
| }; | ||
| scenario.next_tx(RECEIVER); | ||
| { | ||
| let coin = scenario.take_from_address<Coin<VAULT_TESTS>>(RECEIVER); | ||
| assert!(coin.value() == AMOUNT); | ||
| transfer::public_transfer(coin, OWNER); | ||
| }; | ||
| scenario.end(); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add invalid-signature coverage for the settlement paths.
These tests only use signatures produced by solver_keypair(). The suite never proves that fulfil, refund, or mark_for_refund reject a wrong key or malformed signature, so a broken verify_signature path would still leave the core auth boundary green.
Also applies to: 439-476
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sui/rfq-vault/tests/vault_tests.move` around lines 52 - 226, Add negative
coverage for the settlement auth paths in the vault tests by extending the
existing settlement helpers and test cases around fulfil, refund, and
mark_for_refund. Create tests that call vault::fulfil, vault::refund, and
vault::mark_for_refund with signatures that are either signed by the wrong
keypair or otherwise malformed, and assert they abort instead of accepting the
request. Reuse the existing helpers like sign_fulfil, sign_refund,
sign_mark_for_refund, and the Vault/shared setup in vault_tests.move so the
failure cases sit alongside the current positive replay/consumption checks.
Comment on lines
+321
to
+355
| #[test] | ||
| fun owner_can_rescue_and_update_admin_state() { | ||
| let mut scenario = setup_funded_vault(AMOUNT); | ||
| scenario.next_tx(OWNER); | ||
| { | ||
| let mut vault = scenario.take_shared<Vault>(); | ||
| let new_keypair = ecdsa_k1::secp256k1_keypair_from_seed( | ||
| &b"Another random 32 byte test seed", | ||
| ); | ||
|
|
||
| vault::set_solver_pubkey(&mut vault, *new_keypair.public_key(), scenario.ctx()); | ||
| vault::pause(&mut vault, scenario.ctx()); | ||
| assert!(vault::is_paused(&vault)); | ||
| vault::unpause(&mut vault, scenario.ctx()); | ||
| assert!(!vault::is_paused(&vault)); | ||
| vault::rescue<VAULT_TESTS>(&mut vault, AMOUNT, RECEIVER, scenario.ctx()); | ||
| vault::transfer_ownership(&mut vault, OTHER, scenario.ctx()); | ||
|
|
||
| assert!(vault::owner(&vault) == OTHER); | ||
| assert!(vault::vault_balance<VAULT_TESTS>(&vault) == 0); | ||
| test_scenario::return_shared(vault); | ||
| }; | ||
| scenario.end(); | ||
| } | ||
|
|
||
| #[test, expected_failure(abort_code = vault::E_ONLY_OWNER)] | ||
| fun non_owner_cannot_rescue() { | ||
| let mut scenario = setup_funded_vault(AMOUNT); | ||
| scenario.next_tx(OTHER); | ||
| { | ||
| let mut vault = scenario.take_shared<Vault>(); | ||
| vault::rescue<VAULT_TESTS>(&mut vault, AMOUNT, RECEIVER, scenario.ctx()); | ||
| test_scenario::return_shared(vault); | ||
| }; | ||
| scenario.end(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add non-owner abort cases for the other admin entrypoints.
This only proves the happy path for set_solver_pubkey, pause, unpause, and transfer_ownership, while rescue is the only admin path with a negative auth test. A missing owner check on any of those other entrypoints would not be caught here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sui/rfq-vault/tests/vault_tests.move` around lines 321 - 355, The admin-path
coverage in owner_can_rescue_and_update_admin_state only checks the happy path
for set_solver_pubkey, pause, unpause, and transfer_ownership, so add negative
tests proving a non-owner is rejected by each of those Vault entrypoints. Use
the existing non_owner_cannot_rescue pattern as a guide, and add separate
expected_failure cases around vault::set_solver_pubkey, vault::pause,
vault::unpause, and vault::transfer_ownership to ensure the owner-only guard in
Vault is enforced consistently.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/sui/deployRfqVault.ts (1)
64-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead deployment overrides from
process.env.The resolver passes the checked-in constants directly to
requireConfig, so settingSUI_RFQ_VAULT_*environment variables has no effect and the blank defaults still fail. This should resolve env first, then fall back to the config constants.Proposed fix
function requireConfig(name: string, value: string): string { const trimmed = value.trim(); if (!trimmed) { @@ return trimmed; } + +function deploymentValue(name: string, fallback: string): string { + return requireConfig(name, process.env[name] ?? fallback); +} function resolveDeploymentConfig(): DeploymentConfig { const config = { - networkAlias: requireConfig( + networkAlias: deploymentValue( 'SUI_RFQ_VAULT_NETWORK_ALIAS', SUI_RFQ_VAULT_NETWORK_ALIAS, ), - rpcUrl: requireConfig('SUI_RFQ_VAULT_RPC_URL', SUI_RFQ_VAULT_RPC_URL), - ownerAddress: requireConfig( + rpcUrl: deploymentValue('SUI_RFQ_VAULT_RPC_URL', SUI_RFQ_VAULT_RPC_URL), + ownerAddress: deploymentValue( 'SUI_RFQ_VAULT_OWNER_ADDRESS', SUI_RFQ_VAULT_OWNER_ADDRESS, ), - solverPublicKey: requireConfig( + solverPublicKey: deploymentValue( 'SUI_RFQ_VAULT_SOLVER_PUBLIC_KEY', SUI_RFQ_VAULT_SOLVER_PUBLIC_KEY, ), - domain: requireConfig('SUI_RFQ_VAULT_DOMAIN', SUI_RFQ_VAULT_DOMAIN), - gasBudget: requireConfig( + domain: deploymentValue('SUI_RFQ_VAULT_DOMAIN', SUI_RFQ_VAULT_DOMAIN), + gasBudget: deploymentValue( 'SUI_RFQ_VAULT_GAS_BUDGET', SUI_RFQ_VAULT_GAS_BUDGET, ),As per path instructions, “Will another developer easily understand and modify this in 6 months?”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sui/deployRfqVault.ts` around lines 64 - 94, The deployment config resolver is using the checked-in SUI_RFQ_VAULT_* constants directly, so environment overrides are ignored. Update resolveDeploymentConfig to read each value from process.env first and fall back to the imported config constants only when the env var is unset, while keeping requireConfig as the shared validation step. Use the existing requireConfig and resolveDeploymentConfig symbols to wire the env lookup for all fields consistently.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/sui/deployRfqVault.ts`:
- Around line 241-242: Namespace the Sui deployment output by network alias so
`deploymentsPath` no longer points all Sui deployments to the same
`deployments/<stage>/addresses/sui.json` file. Update the `deploymentsPath`
helper and the call sites in `deployRfqVault.ts` to include the Sui network
alias in the path or key namespace, matching the per-network persistence pattern
used by the RFQ vault executor helper, so separate aliases do not overwrite each
other’s package/vault IDs.
- Around line 383-410: The deployment flow in deployRfqVault.ts persists state
too late: the optional verify-source step runs before persistDeployment, so a
verification failure leaves the successful publish/create_vault results
unrecorded. Move the persistDeployment call to immediately after
createVaultResult is available, before the config.verifySource block, and keep
using the existing packageId, vaultId, upgradeCapId, and digest fields from
publishResult/createVaultResult. Then run runSui for source verification after
the IDs have already been saved so reruns don’t create duplicate deployments.
- Around line 296-310: The key import in deployRfqVault.ts passes
deployer.secret directly to runSui as a command-line argument, which exposes it
in argv. Update the keytool import flow to use an import path or stdin-based
mechanism that keeps the secret out of the command line, and keep the change
localized around the runSui invocation that imports into the temporary keystore
with DEPLOYER_ALIAS.
---
Nitpick comments:
In `@scripts/sui/deployRfqVault.ts`:
- Around line 64-94: The deployment config resolver is using the checked-in
SUI_RFQ_VAULT_* constants directly, so environment overrides are ignored. Update
resolveDeploymentConfig to read each value from process.env first and fall back
to the imported config constants only when the env var is unset, while keeping
requireConfig as the shared validation step. Use the existing requireConfig and
resolveDeploymentConfig symbols to wire the env lookup for all fields
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 450c9329-e676-484c-b937-d4ddd61e29c9
📒 Files selected for processing (4)
.gitignorepackage.jsonscripts/sui/deployRfqVault.tsscripts/sui/rfqVaultDeploymentConfig.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
Comment on lines
+241
to
+242
| function deploymentsPath(stage: string): string { | ||
| return resolve(REPO_ROOT, 'deployments', stage, 'addresses', 'sui.json'); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Namespace deployment records by Sui network alias.
deploymentsPath always writes fixed keys into deployments/<stage>/addresses/sui.json, so a second Sui deployment for another alias overwrites the previous package/vault IDs. This should include the network alias in the path or namespace the keys; the existing RFQ vault executor helper persists per-network address files.
Proposed fix
-function deploymentsPath(stage: string): string {
- return resolve(REPO_ROOT, 'deployments', stage, 'addresses', 'sui.json');
+function deploymentsPath(stage: string, networkAlias: string): string {
+ return resolve(
+ REPO_ROOT,
+ 'deployments',
+ stage,
+ 'addresses',
+ `${networkAlias}.json`,
+ );
}As per path instructions, “Check for resource leaks, race conditions, and unhandled edge cases.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function deploymentsPath(stage: string): string { | |
| return resolve(REPO_ROOT, 'deployments', stage, 'addresses', 'sui.json'); | |
| function deploymentsPath(stage: string, networkAlias: string): string { | |
| return resolve( | |
| REPO_ROOT, | |
| 'deployments', | |
| stage, | |
| 'addresses', | |
| `${networkAlias}.json`, | |
| ); |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/sui/deployRfqVault.ts` around lines 241 - 242, Namespace the Sui
deployment output by network alias so `deploymentsPath` no longer points all Sui
deployments to the same `deployments/<stage>/addresses/sui.json` file. Update
the `deploymentsPath` helper and the call sites in `deployRfqVault.ts` to
include the Sui network alias in the path or key namespace, matching the
per-network persistence pattern used by the RFQ vault executor helper, so
separate aliases do not overwrite each other’s package/vault IDs.
Source: Path instructions
Comment on lines
+296
to
+310
| runSui( | ||
| [ | ||
| 'keytool', | ||
| 'import', | ||
| '--alias', | ||
| DEPLOYER_ALIAS, | ||
| '--json', | ||
| deployer.secret, | ||
| deployer.scheme, | ||
| ], | ||
| { | ||
| env, | ||
| label: 'import SUI_DEPLOYER into temporary keystore', | ||
| redactions: [deployer.secret], | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which non-argv import modes are supported by the installed Sui CLI.
sui keytool import --helpRepository: SocketDotTech/openrouter
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the deploy script and the Sui command wrapper.
git ls-files 'scripts/sui/deployRfqVault.ts' 'scripts/**' | sed -n '1,120p'
echo '--- deployRfqVault.ts outline ---'
ast-grep outline scripts/sui/deployRfqVault.ts --view expanded
echo '--- runSui references ---'
rg -n "runSui\(" scripts/sui -S
echo '--- keytool import usage ---'
rg -n "keytool import|import --alias|DEPLOYER_ALIAS|secret" scripts/sui -SRepository: SocketDotTech/openrouter
Length of output: 8007
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section and the helper that spawns the child process.
sed -n '158,320p' scripts/sui/deployRfqVault.ts | cat -n
echo '---'
sed -n '320,420p' scripts/sui/deployRfqVault.ts | cat -nRepository: SocketDotTech/openrouter
Length of output: 8664
🌐 Web query:
Sui CLI keytool import supports stdin file alias secret argv
💡 Result:
The sui keytool import command is used to add a new key to the Sui CLI keystore [1][2]. Regarding the specific aspects queried: 1. Stdin: The command does not natively support passing the mnemonic phrase or private key via stdin (piping). The input string must be provided as a positional argument [1][2]. 2. File: The command does not natively support importing a key directly from a file path as an argument. The mnemonic phrase or Bech32-encoded key must be provided as a string argument [1][2]. 3. Alias: The command supports an optional --alias flag to set a human-readable name for the imported key [1][3]. If no alias is provided, the CLI will automatically generate one [1][2]. A bug that caused the --alias flag to be ignored when importing via mnemonic was fixed in 2024 [4]. 4. Secret: The command accepts the secret key in two formats: as a mnemonic phrase (12, 15, 18, 21, or 24 words) or as a Bech32-encoded string (33-byte flag || private key) starting with suiprivkey [1][2]. The secret string must be enclosed in single or double quotes in the CLI to prevent shell interpretation [5]. 5. Argv: The command expects the input string (mnemonic or suiprivkey) and the key scheme (e.g., ed25519, secp256k1, or secp256r1) as positional command-line arguments, with an optional derivation path as a final positional argument [1][2]. The standard usage syntax is: sui keytool import [OPTIONS] <INPUT_STRING> <KEY_SCHEME> [DERIVATION_PATH] [2]
Citations:
- 1: https://docs.sui.io/references/cli/keytool
- 2: https://cookbook.sui-book.com/01_sui_cli/03.keytool.html
- 3: https://github.com/MystenLabs/sui/blob/4af8007c/crates/sui/src/keytool.rs
- 4: [CLI] fix command
sui keytool importusing--aliasis useless when inputting mnemonic phrase MystenLabs/sui#17935 - 5: https://docs.sui.io/getting-started/onboarding/get-address
Avoid passing deployer.secret on the command line.
keytool import puts the secret in argv, which exposes it to local process listings; this needs an import path that keeps the secret out of the command line.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/sui/deployRfqVault.ts` around lines 296 - 310, The key import in
deployRfqVault.ts passes deployer.secret directly to runSui as a command-line
argument, which exposes it in argv. Update the keytool import flow to use an
import path or stdin-based mechanism that keeps the secret out of the command
line, and keep the change localized around the runSui invocation that imports
into the temporary keystore with DEPLOYER_ALIAS.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit