Sui rfq deploy#12
Conversation
WalkthroughIntroduces a complete Sui Move RFQ vault contract ( ChangesSui RFQ Vault Move Contract
Deployment Scripts and Configuration
Sequence Diagram(s)sequenceDiagram
participant Solver
participant Caller
participant Vault
participant ecdsa_k1
rect rgba(100, 149, 237, 0.5)
note over Caller,Vault: Deposit
Caller->>Vault: deposit(quote_id, coin)
Vault->>Vault: assert_not_paused, assert_valid_quote_id
Vault->>Vault: balance_mut_or_create~T~, join amount
Vault-->>Caller: emit Deposited
end
rect rgba(144, 238, 144, 0.5)
note over Solver,Vault: Fulfil
Solver->>Vault: fulfil(quote_id, nonce, amount, receiver, signature)
Vault->>Vault: build fulfil_message bytes (BCS)
Vault->>ecdsa_k1: secp256k1_verify(sig, msg, solver_pubkey, SHA256)
ecdsa_k1-->>Vault: ok / abort E_INVALID_SIGNATURE
Vault->>Vault: mark_quote_id (abort if reused)
Vault->>Vault: existing_balance_mut~T~, split coin
Vault-->>Solver: transfer coin to receiver, emit Fulfilled
end
rect rgba(255, 160, 122, 0.5)
note over Solver,Vault: Refund path
Solver->>Vault: mark_for_refund(quote_id, nonce, signature)
Vault->>ecdsa_k1: secp256k1_verify
Vault->>Vault: mark quote_used + marked_for_refund
Solver->>Vault: refund(quote_id, nonce, amount, receiver, signature)
Vault->>ecdsa_k1: secp256k1_verify
Vault->>Vault: mark_quote_id (abort if reused)
Vault-->>Solver: transfer coin to receiver, emit Refunded
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
🤖 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 423-448: Move the deployment persistence in deployRfqVault so the
record is written only after source verification passes. The current
persistDeployment call in deployRfqVault runs before the config.verifySource
branch and can save an unverified package; relocate that call to after the
runSui('verify-source', ...) step succeeds, keeping the same deployment fields
and using the existing packageId/vaultId data.
- Around line 316-331: The deployer secret is being passed as a positional
argument to the `runSui` call that invokes `keytool import`, which exposes it
through argv/process listings. Update the `deployRfqVault` flow to avoid passing
`deployer.secret` on the command line by using a non-argv import path such as a
preloaded isolated keystore or another supported stdin/file-based mechanism,
while keeping the existing `DEPLOYER_ALIAS` import behavior intact.
In `@sui/rfq-vault/sources/vault.move`:
- Around line 115-116: The deposit flow in vault.move currently only checks
assert_not_paused and assert_valid_quote_id, so it can still accept a quote_id
that has already been fulfilled, refunded, or marked for refund. Update the
deposit entry path in the vault module to reject any consumed quote_id by adding
a single-use state check alongside assert_valid_quote_id, using the existing
quote lifecycle helpers/state transitions used elsewhere in the module. Ensure
the deposit function aborts before funds are accepted when the quote_id is no
longer active.
🪄 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: f4f4af8d-59e6-4a99-a2cf-3aeb6e880273
⛔ Files ignored due to path filters (1)
sui/rfq-vault/Move.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.gitignoredeployments/prod/addresses/sui.jsonpackage.jsonscripts/sui/deployRfqVault.tsscripts/sui/printKmsSolverKey.tsscripts/sui/rfqVaultDeploymentConfig.tssui/rfq-vault/Move.tomlsui/rfq-vault/Published.tomlsui/rfq-vault/sources/vault.movesui/rfq-vault/tests/vault_tests.move
| 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
set -euo pipefail
if ! command -v sui >/dev/null 2>&1; then
echo "sui CLI not installed here; check 'sui keytool import --help' locally for stdin/file/prompt options."
exit 0
fi
sui keytool import --help | sed -n '1,200p'Repository: SocketDotTech/openrouter
Length of output: 263
🌐 Web query:
Sui CLI keytool import help stdin file prompt import secret keytool import --help documentation
💡 Result:
The sui keytool import command is used to add a new key to the Sui CLI Keystore [1][2]. The command accepts either a mnemonic phrase or a Bech32-encoded private key starting with the prefix suiprivkey [1][3]. Usage and Syntax The basic syntax for the command is: sui keytool import '' <KEY_SCHEME> Parameters: INPUT: A mnemonic phrase (e.g., 12, 15, 18, 21, or 24 words) or a Bech32-encoded 33-byte flag || privkey string starting with suiprivkey [1][4]. KEY_SCHEME: The signature scheme, which must be one of ed25519, secp256k1, or secp256r1 [1][4]. Important Notes: 1. Handling Stdin and Prompting: The Sui CLI does not natively support an explicit flag to pipe a secret key from stdin into the import command [4]. It expects the input string as a positional argument [4]. When using a mnemonic phrase, it is recommended to wrap the entire phrase in single quotes to ensure it is processed correctly as a single argument [5]. 2. Hex Format Deprecation: Sui no longer supports importing private keys in Hex format [6][7]. If you have a legacy Hex-encoded private key, you must first convert it to the required Bech32 format using the sui keytool convert <HEX_KEY> command [6][7]. 3. Aliases: You can assign an alias to the imported key using the --alias flag [1][4]. If no alias is provided, the tool will automatically generate one [1][4]. 4. Help Documentation: To view the full command help and options directly in your terminal, run: sui keytool import --help
Citations:
- 1: https://docs.sui.io/references/cli/keytool
- 2: https://docs.sui.io/doc/sui-cli-cheatsheet.pdf
- 3: https://docs.sui.io/references/cli/cheatsheet
- 4: https://github.com/MystenLabs/sui/blob/4af8007c/crates/sui/src/keytool.rs
- 5: https://docs.sui.io/getting-started/onboarding/get-address
- 6: feat: unify import/export private key format MystenLabs/sui#15415
- 7: https://github.com/sui-foundation/sips/blob/main/sips/sip-15.md
Keep the deployer secret out of argv. Line 323 passes deployer.secret to sui keytool import as a positional argument, and that interface exposes the secret in process listings/procfs on shared CI or hosts. This should use a preloaded isolated keystore or another non-argv import path.
🧰 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 316 - 331, The deployer secret is
being passed as a positional argument to the `runSui` call that invokes `keytool
import`, which exposes it through argv/process listings. Update the
`deployRfqVault` flow to avoid passing `deployer.secret` on the command line by
using a non-argv import path such as a preloaded isolated keystore or another
supported stdin/file-based mechanism, while keeping the existing
`DEPLOYER_ALIAS` import behavior intact.
| const filePath = await persistDeployment(config.stage, { | ||
| SuiRFQVaultPackage: packageId, | ||
| SuiRFQVault: vaultId, | ||
| SuiRFQVaultUpgradeCap: upgradeCapId, | ||
| SuiRFQVaultPublishTx: publishResult.digest, | ||
| SuiRFQVaultUpgradeCapTransferTx: upgradeCapTransferDigest, | ||
| SuiRFQVaultCreateTx: createVaultResult.digest, | ||
| SuiRFQVaultOwner: config.ownerAddress, | ||
| SuiRFQVaultSolverPublicKey: config.solverPublicKey, | ||
| SuiRFQVaultDomain: config.domain, | ||
| SuiRFQVaultNetworkAlias: config.networkAlias, | ||
| }); | ||
|
|
||
| if (config.verifySource) { | ||
| runSui( | ||
| [ | ||
| 'client', | ||
| '--client.config', | ||
| clientConfig, | ||
| '--json', | ||
| 'verify-source', | ||
| packagePath, | ||
| '--verify-deps', | ||
| ], | ||
| { env, label: 'verify published source' }, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the deployment record after verification succeeds.
Line 423 writes deployments/prod/addresses/sui.json before Line 436 runs source verification. A failed verification still leaves an updated production address record for an unverified package. This should move persistDeployment below the verification gate.
Proposed ordering fix
- const filePath = await persistDeployment(config.stage, {
- SuiRFQVaultPackage: packageId,
- SuiRFQVault: vaultId,
- SuiRFQVaultUpgradeCap: upgradeCapId,
- SuiRFQVaultPublishTx: publishResult.digest,
- SuiRFQVaultUpgradeCapTransferTx: upgradeCapTransferDigest,
- SuiRFQVaultCreateTx: createVaultResult.digest,
- SuiRFQVaultOwner: config.ownerAddress,
- SuiRFQVaultSolverPublicKey: config.solverPublicKey,
- SuiRFQVaultDomain: config.domain,
- SuiRFQVaultNetworkAlias: config.networkAlias,
- });
-
if (config.verifySource) {
runSui(
[
@@
{ env, label: 'verify published source' },
);
}
+
+ const filePath = await persistDeployment(config.stage, {
+ SuiRFQVaultPackage: packageId,
+ SuiRFQVault: vaultId,
+ SuiRFQVaultUpgradeCap: upgradeCapId,
+ SuiRFQVaultPublishTx: publishResult.digest,
+ SuiRFQVaultUpgradeCapTransferTx: upgradeCapTransferDigest,
+ SuiRFQVaultCreateTx: createVaultResult.digest,
+ SuiRFQVaultOwner: config.ownerAddress,
+ SuiRFQVaultSolverPublicKey: config.solverPublicKey,
+ SuiRFQVaultDomain: config.domain,
+ SuiRFQVaultNetworkAlias: config.networkAlias,
+ });📝 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.
| const filePath = await persistDeployment(config.stage, { | |
| SuiRFQVaultPackage: packageId, | |
| SuiRFQVault: vaultId, | |
| SuiRFQVaultUpgradeCap: upgradeCapId, | |
| SuiRFQVaultPublishTx: publishResult.digest, | |
| SuiRFQVaultUpgradeCapTransferTx: upgradeCapTransferDigest, | |
| SuiRFQVaultCreateTx: createVaultResult.digest, | |
| SuiRFQVaultOwner: config.ownerAddress, | |
| SuiRFQVaultSolverPublicKey: config.solverPublicKey, | |
| SuiRFQVaultDomain: config.domain, | |
| SuiRFQVaultNetworkAlias: config.networkAlias, | |
| }); | |
| if (config.verifySource) { | |
| runSui( | |
| [ | |
| 'client', | |
| '--client.config', | |
| clientConfig, | |
| '--json', | |
| 'verify-source', | |
| packagePath, | |
| '--verify-deps', | |
| ], | |
| { env, label: 'verify published source' }, | |
| ); | |
| if (config.verifySource) { | |
| runSui( | |
| [ | |
| 'client', | |
| '--client.config', | |
| clientConfig, | |
| '--json', | |
| 'verify-source', | |
| packagePath, | |
| '--verify-deps', | |
| ], | |
| { env, label: 'verify published source' }, | |
| ); | |
| } | |
| const filePath = await persistDeployment(config.stage, { | |
| SuiRFQVaultPackage: packageId, | |
| SuiRFQVault: vaultId, | |
| SuiRFQVaultUpgradeCap: upgradeCapId, | |
| SuiRFQVaultPublishTx: publishResult.digest, | |
| SuiRFQVaultUpgradeCapTransferTx: upgradeCapTransferDigest, | |
| SuiRFQVaultCreateTx: createVaultResult.digest, | |
| SuiRFQVaultOwner: config.ownerAddress, | |
| SuiRFQVaultSolverPublicKey: config.solverPublicKey, | |
| SuiRFQVaultDomain: config.domain, | |
| SuiRFQVaultNetworkAlias: config.networkAlias, | |
| }); |
🧰 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 423 - 448, Move the deployment
persistence in deployRfqVault so the record is written only after source
verification passes. The current persistDeployment call in deployRfqVault runs
before the config.verifySource branch and can save an unverified package;
relocate that call to after the runSui('verify-source', ...) step succeeds,
keeping the same deployment fields and using the existing packageId/vaultId
data.
| assert_not_paused(vault); | ||
| assert_valid_quote_id("e_id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject deposits for consumed quote IDs.
This should abort once a quote_id has already been fulfilled, refunded, or marked for refund. The rest of the module treats quote_id as single-use, so accepting a later deposit strands funds in the pooled balance with no matching settlement path.
Proposed fix
assert_not_paused(vault);
assert_valid_quote_id("e_id);
+ assert!(
+ !table::contains<vector<u8>, bool>(&vault.quote_used, copy quote_id),
+ E_INVALID_QUOTE_ID,
+ );
let amount = coin::value<T>(&coin);📝 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.
| assert_not_paused(vault); | |
| assert_valid_quote_id("e_id); | |
| assert_not_paused(vault); | |
| assert_valid_quote_id("e_id); | |
| assert!( | |
| !table::contains<vector<u8>, bool>(&vault.quote_used, copy quote_id), | |
| E_INVALID_QUOTE_ID, | |
| ); |
🤖 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 115 - 116, The deposit flow in
vault.move currently only checks assert_not_paused and assert_valid_quote_id, so
it can still accept a quote_id that has already been fulfilled, refunded, or
marked for refund. Update the deposit entry path in the vault module to reject
any consumed quote_id by adding a single-use state check alongside
assert_valid_quote_id, using the existing quote lifecycle helpers/state
transitions used elsewhere in the module. Ensure the deposit function aborts
before funds are accepted when the quote_id is no longer active.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes