-
Notifications
You must be signed in to change notification settings - Fork 60
Add test for SMT root replay (#211) #236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8a43bd4
87974a4
dffca31
d295cdc
9216382
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| name: SDK Tests | ||
|
|
||
| on: | ||
| push: | ||
| branches: ["main"] | ||
| pull_request: | ||
| branches: ["main"] | ||
|
|
||
| jobs: | ||
| sdk-tests: | ||
| runs-on: ubuntu-latest | ||
| defaults: | ||
| run: | ||
| working-directory: zk/sdk | ||
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20 | ||
|
|
||
| - name: Install dependencies | ||
| run: npm install | ||
|
|
||
| - name: Run SDK tests | ||
| run: npm test |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # Core Contract Notes | ||
|
|
||
| ## SMT Root Sequencing Requirement | ||
|
|
||
| `register_resolver` enforces strict root sequencing: | ||
|
|
||
| - `public_signals.old_root` must exactly equal the current on-chain SMT root. | ||
| - A successful `register_resolver` updates the on-chain root to `public_signals.new_root`. | ||
| - Any later call reusing the pre-update root is rejected as stale. | ||
|
|
||
| This replay protection prevents re-submitting proofs against an already-consumed root. In tests, the stale replay path is asserted to panic with `Error(Contract, #4)` (`StaleRoot`). | ||
|
|
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -103,6 +103,39 @@ impl EscrowContract { | |||||||||||||||
| Events::vault_crt(&env, commitment, token, owner); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// Deposits tokens into an active vault. | ||||||||||||||||
| /// | ||||||||||||||||
| /// The vault owner must authorize the call. Funds are transferred from the | ||||||||||||||||
| /// owner to this contract and reflected in the vault's tracked balance. | ||||||||||||||||
| /// | ||||||||||||||||
| /// ### Errors | ||||||||||||||||
| /// - `InvalidAmount`: If `amount <= 0`. | ||||||||||||||||
| /// - `VaultNotFound`: If the vault does not exist. | ||||||||||||||||
| /// - `VaultInactive`: If the vault has been cancelled/inactivated. | ||||||||||||||||
| pub fn deposit(env: Env, commitment: BytesN<32>, amount: i128) { | ||||||||||||||||
| if amount <= 0 { | ||||||||||||||||
| panic_with_error!(&env, EscrowError::InvalidAmount); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| let config = read_vault_config(&env, &commitment) | ||||||||||||||||
| .unwrap_or_else(|| panic_with_error!(&env, EscrowError::VaultNotFound)); | ||||||||||||||||
| config.owner.require_auth(); | ||||||||||||||||
|
|
||||||||||||||||
| let mut state = read_vault_state(&env, &commitment) | ||||||||||||||||
| .unwrap_or_else(|| panic_with_error!(&env, EscrowError::VaultNotFound)); | ||||||||||||||||
| if !state.is_active { | ||||||||||||||||
| panic_with_error!(&env, EscrowError::VaultInactive); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| let token_client = token::Client::new(&env, &config.token); | ||||||||||||||||
| token_client.transfer(&config.owner, &env.current_contract_address(), &amount); | ||||||||||||||||
|
Comment on lines
+130
to
+131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find all needless-borrow candidates for current_contract_address.
rg -n --type rust '&env\.current_contract_address\(\)'Repository: Alien-Protocol/Alien-Gateway Length of output: 869 🏁 Script executed: # First, let's look at the escrow_contract lib.rs around line 131 to understand the context
sed -n '125,135p' gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway Length of output: 507 🏁 Script executed: # Let's also check the full scope of the transfer calls and token client setup
sed -n '125,155p' gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway Length of output: 1340 🏁 Script executed: # Check what token module is being used - look for imports
head -50 gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway Length of output: 1862 🏁 Script executed: # Check if there's any custom token module definition in the codebase
fd -e rs | xargs rg -l 'impl.*transfer' | head -20Repository: Alien-Protocol/Alien-Gateway Length of output: 54 🏁 Script executed: # Look for soroban-sdk version in Cargo.toml to understand what version is being used
find . -name "Cargo.toml" -exec grep -H "soroban-sdk" {} \;Repository: Alien-Protocol/Alien-Gateway Length of output: 1049 🏁 Script executed: # Check the token client type to understand what transfer expects
rg -A5 'token::Client::new' gateway-contract/contracts/escrow_contract/src/lib.rsRepository: Alien-Protocol/Alien-Gateway Length of output: 1028 🏁 Script executed: # Search for soroban-sdk documentation or check what Address type is
# Let's look for any type definitions or imports that might give us clues
rg 'Address' gateway-contract/contracts/escrow_contract/src/lib.rs -B2 -A2 | head -30Repository: Alien-Protocol/Alien-Gateway Length of output: 1330 🌐 Web query:
💡 Result: The transfer method signature of soroban_sdk::token::TokenClient is: rust pub fn transfer( &self, from: &Address, to: impl Into, amount: &i128, ) The first parameter is from: &Address, which specifies the source address holding the tokens to be transferred. The second parameter to accepts values convertible into MuxedAddress (including Address), representing the destination. This is confirmed in the official Soroban SDK documentation for TokenClient. Citations:
🏁 Script executed: # Let's check if there are compilation errors by looking at CI logs or checking other transfer patterns
# First, check all token_client.transfer calls in the codebase to see patterns
rg -B2 -A2 'token_client\.transfer\(' gateway-contract/contracts/Repository: Alien-Protocol/Alien-Gateway Length of output: 2066 🏁 Script executed: # Let's check if this matches a Soroban SDK pattern issue
# Look at how Address is typically used as a parameter
rg 'to: &env' gateway-contract/ -A1 -B1Repository: Alien-Protocol/Alien-Gateway Length of output: 54 Remove needless borrow in token transfer call. Line 131 should pass Proposed fix- token_client.transfer(&config.owner, &env.current_contract_address(), &amount);
+ token_client.transfer(&config.owner, env.current_contract_address(), &amount);🧰 Tools🪛 GitHub Actions: Checks[error] 131-131: Clippy failed: 🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
| state.balance += amount; | ||||||||||||||||
| write_vault_state(&env, &commitment, &state); | ||||||||||||||||
|
Comment on lines
+133
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify whether balance updates in escrow use checked arithmetic.
rg -n --type rust 'balance\s*[\+\-]=|checked_add\(|checked_sub\(' gateway-contract/contracts/escrow_contract/srcRepository: Alien-Protocol/Alien-Gateway Length of output: 524 🏁 Script executed: cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '125,145p'Repository: Alien-Protocol/Alien-Gateway Length of output: 1043 🏁 Script executed: cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '185,205p'Repository: Alien-Protocol/Alien-Gateway Length of output: 790 🏁 Script executed: cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '400,415p'Repository: Alien-Protocol/Alien-Gateway Length of output: 647 🏁 Script executed: rg -n "struct.*State|balance.*:" gateway-contract/contracts/escrow_contract/src --type rust -A 3 | head -30Repository: Alien-Protocol/Alien-Gateway Length of output: 2120 🏁 Script executed: cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '370,410p'Repository: Alien-Protocol/Alien-Gateway Length of output: 1864 🏁 Script executed: cat -n gateway-contract/contracts/escrow_contract/src/lib.rs | sed -n '100,138p'Repository: Alien-Protocol/Alien-Gateway Length of output: 1852 Fix unchecked addition overflow in vault balance deposit. Line 133 uses unchecked addition ( Use checked arithmetic to detect and handle overflow: Proposed fix- state.balance += amount;
+ state.balance = state
+ .balance
+ .checked_add(amount)
+ .unwrap_or_else(|| panic_with_error!(&env, EscrowError::InvalidAmount));
write_vault_state(&env, &commitment, &state);📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
| Events::deposit(&env, commitment, config.owner, amount, state.balance); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /// Schedules a payment from one vault to another. | ||||||||||||||||
| /// | ||||||||||||||||
| /// Funds are reserved in the source vault immediately upon scheduling. | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,102 @@ fn read_vault(env: &Env, contract_id: &Address, id: &BytesN<32>) -> VaultState { | |
| }) | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_deposit_success() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
| let (contract_id, client, token, _token_admin, from, _to) = setup_test(&env); | ||
|
|
||
| let owner = Address::generate(&env); | ||
| create_vault(&env, &contract_id, &from, &owner, &token, 0); | ||
|
|
||
| let token_admin_client = StellarAssetClient::new(&env, &token); | ||
| token_admin_client.mint(&owner, &500); | ||
|
|
||
| client.deposit(&from, &200); | ||
|
|
||
| env.as_contract(&contract_id, || { | ||
| let state: VaultState = env | ||
| .storage() | ||
| .persistent() | ||
| .get(&DataKey::VaultState(from.clone())) | ||
| .unwrap(); | ||
| assert_eq!(state.balance, 200); | ||
| }); | ||
|
|
||
| let token_client = TokenClient::new(&env, &token); | ||
| assert_eq!(token_client.balance(&owner), 300); | ||
| assert_eq!(token_client.balance(&contract_id), 200); | ||
|
|
||
| let deposit_events = env | ||
| .events() | ||
| .all() | ||
| .iter() | ||
| .filter(|(event_contract, _, _)| event_contract == &contract_id) | ||
| .count(); | ||
| assert_eq!(deposit_events, 1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_deposit_zero_amount_rejected() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
| let (contract_id, client, token, _token_admin, from, _to) = setup_test(&env); | ||
|
|
||
| create_vault( | ||
| &env, | ||
| &contract_id, | ||
| &from, | ||
| &Address::generate(&env), | ||
| &token, | ||
| 0, | ||
| ); | ||
|
|
||
| let result = client.try_deposit(&from, &0); | ||
| assert!(matches!( | ||
| result, | ||
| Err(Ok(err)) if err == Error::from_contract_error(EscrowError::InvalidAmount as u32) | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| #[should_panic] | ||
| fn test_deposit_non_owner_panics() { | ||
| let env = Env::default(); | ||
| let (contract_id, client, token, _token_admin, from, _to) = setup_test(&env); | ||
|
|
||
| let owner = Address::generate(&env); | ||
| create_vault(&env, &contract_id, &from, &owner, &token, 0); | ||
|
|
||
| client.deposit(&from, &10); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_deposit_inactive_vault_rejected() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
| let (contract_id, client, token, _token_admin, from, _to) = setup_test(&env); | ||
|
|
||
| let owner = Address::generate(&env); | ||
| create_vault(&env, &contract_id, &from, &owner, &token, 0); | ||
|
|
||
| env.as_contract(&contract_id, || { | ||
| let state = VaultState { | ||
| balance: 0, | ||
| is_active: false, | ||
| }; | ||
| env.storage() | ||
| .persistent() | ||
| .set(&DataKey::VaultState(from.clone()), &state); | ||
| }); | ||
|
|
||
| let result = client.try_deposit(&from, &10); | ||
| assert!(matches!( | ||
| result, | ||
| Err(Ok(err)) if err == Error::from_contract_error(EscrowError::VaultInactive as u32) | ||
| )); | ||
| } | ||
|
|
||
|
Comment on lines
+88
to
+183
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR objective mismatch: required SMT stale-root replay test/docs are not covered in this diff. Issue 🤖 Prompt for AI Agents |
||
| #[test] | ||
| fn test_schedule_payment_success() { | ||
| let env = Env::default(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| const config = { | ||
| preset: "ts-jest", | ||
| testEnvironment: "node", | ||
| roots: ["<rootDir>/src"], | ||
| testMatch: ["**/__tests__/**/*.test.ts"], | ||
| clearMocks: true, | ||
| }; | ||
|
|
||
| export default config; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,4 +6,4 @@ | |
| "dependencies": { | ||
| "circomlibjs": "^0.1.7" | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import * as sdk from "../index"; | ||
|
|
||
| declare const describe: (name: string, fn: () => void) => void; | ||
| declare const it: (name: string, fn: () => void) => void; | ||
| declare const expect: (value: unknown) => { toBeDefined: () => void }; | ||
|
|
||
| describe("zk sdk smoke", () => { | ||
| it("imports index without throwing", () => { | ||
| expect(sdk).toBeDefined(); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.