Fix/escrow test read vault key#219
Open
olaleyeolajide81-sketch wants to merge 4 commits intoAlien-Protocol:mainfrom
Open
Fix/escrow test read vault key#219olaleyeolajide81-sketch wants to merge 4 commits intoAlien-Protocol:mainfrom
olaleyeolajide81-sketch wants to merge 4 commits intoAlien-Protocol:mainfrom
Conversation
added 3 commits
March 26, 2026 09:04
…taKey::Vault causing storage mismatch - Update DataKey enum to include VaultState and VaultConfig variants - Fix read_vault() to read from DataKey::VaultState instead of legacy DataKey::Vault - Fix write_vault() to use DataKey::VaultState for consistency - Update create_vault() test helper to use DataKey::VaultState - Fix all direct DataKey::Vault usages in tests to use DataKey::VaultState - Mark legacy DataKey::Vault as deprecated for backward compatibility Resolves Alien-Protocol#178
|
@olaleyeolajide81-sketch Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughTransitions from username-leaf circuit testing infrastructure to implementing a read-only escrow contract getter Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🧹 Nitpick comments (1)
zk/verify_implementation.js (1)
52-52: Silent failure paths reduce debuggability in CI.Lines 52, 70-72, and 84-86 exit immediately without a failure reason. Keep logs minimal, but print a single stderr reason per failing check to make CI failures actionable.
Proposed minimal diagnostic refactor
+function fail(msg) { + process.stderr.write(`${msg}\n`); + process.exit(1); +} ... - if (!matches) { - process.exit(1); - } + if (!matches) fail('input.json username encoding mismatch'); ... - if (!exists) { - process.exit(1); - } + if (!exists) fail(`Missing package.json script: ${script}`); ... - if (!hasUsernameLeaf) { - process.exit(1); - } + if (!hasUsernameLeaf) fail('scripts/compile.sh missing username_leaf_main');Also applies to: 70-72, 84-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/verify_implementation.js` at line 52, Several failing checks currently call process.exit(1) with no message; update each site that calls process.exit(1) (the bare process.exit(1) invocations) to first write a single minimal diagnostic to stderr via console.error(...) describing which check failed (one short phrase per failure) and then call process.exit(1); keep messages concise and specific to the check (e.g., "verification setup failed", "proof missing", "ABI mismatch") so CI logs show one clear reason per early exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PR_DESCRIPTION.md`:
- Around line 23-29: Replace the replacement characters in the two PR headings
by editing the headings "### � CI/CD Fixes" and "### �📚 Documentation" to valid
UTF-8 characters (e.g., use a proper emoji like "⚙️ CI/CD Fixes" and "📚
Documentation" or remove the emoji entirely) and ensure the file is saved with
UTF-8 encoding so the characters render correctly; update only those heading
lines to the corrected text.
- Around line 1-8: The PR metadata in PR_DESCRIPTION.md is inconsistent: the
title and Summary mention get_balance and close issue `#74`, but the actual
objective is the escrow test storage-key fix (issue `#178`). Update the title
line, the Summary paragraph text, and the closure reference so they all describe
the escrow test storage-key fix and use "Closes `#178`" (replace any references to
get_balance and "#74"); ensure the summary briefly states the storage-key test
fix and references the escrow contract change to keep tracking accurate.
- Around line 112-117: Update the mismatch in the PR description by making the
"Modified Files (1)" count consistent with the enumerated file list: either
change the header number to reflect the actual number of modified files (e.g.,
"Modified Files (4)") or remove the extra file entries so only the single
modified file (`gateway-contract/contracts/escrow_contract/src/lib.rs`) remains;
edit the PR_DESCRIPTION.md section containing the "Modified Files (1)" header
and the following bullet list to keep the count and the list synchronized.
- Around line 42-47: The doc snippet uses the wrong storage reader; update
get_balance to call the storage function read_vault_state(...) (which returns
Option<VaultState>) instead of read_vault, and unwrap the returned
Option<VaultState> to access .balance or return 0 when None; reference
get_balance and read_vault_state to locate and replace the call and ensure the
types align with VaultState.
In `@zk/verify_implementation.js`:
- Around line 83-86: The verification script is failing because
verify_implementation.js looks for "username_leaf_main" but the CIRCUITS array
in compile.sh does not include that circuit; fix by adding the entry
"username_leaf_main|merkle/username_leaf_main.circom" to the CIRCUITS array in
compile.sh so the circuit is compiled (preferred), or if that circuit should not
be compiled, update the include check in verify_implementation.js to remove or
adjust the "username_leaf_main" presence check so it matches the actual compiled
CIRCUITS array; reference symbols: CIRCUITS array, "username_leaf_main",
compile.sh, verify_implementation.js.
---
Nitpick comments:
In `@zk/verify_implementation.js`:
- Line 52: Several failing checks currently call process.exit(1) with no
message; update each site that calls process.exit(1) (the bare process.exit(1)
invocations) to first write a single minimal diagnostic to stderr via
console.error(...) describing which check failed (one short phrase per failure)
and then call process.exit(1); keep messages concise and specific to the check
(e.g., "verification setup failed", "proof missing", "ABI mismatch") so CI logs
show one clear reason per early exit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cbfffd7-ac7e-4e79-8f8f-f0f215855d64
📒 Files selected for processing (3)
PR_DESCRIPTION.mdzk/tests/username_leaf_test.tszk/verify_implementation.js
💤 Files with no reviewable changes (1)
- zk/tests/username_leaf_test.ts
Comment on lines
+1
to
8
| # Pull Request: [Contract] Escrow — implement get_balance(commitment) read-only getter | ||
|
|
||
| ## Summary | ||
| Implements comprehensive standalone testing for the `username_leaf.circom` component, ensuring it can be independently verified as the bridge between raw username arrays and Poseidon-hashed Merkle leaves. | ||
| Implements `get_balance` — a public read-only entry point that returns the current token balance of a vault. Used by the SDK and frontend dashboard to display vault state without triggering authentication. | ||
|
|
||
| ## Issue Addressed | ||
| Closes #68: [ZK] Username Leaf Circuit — extract and test username_leaf.circom as standalone component | ||
| Closes #74: [Contract] Escrow — implement get_balance(commitment) read-only getter | ||
|
|
There was a problem hiding this comment.
PR metadata does not match the actual objective/issue linkage.
Line 1 and Line 7 describe get_balance/issue #74, while this PR’s stated objective is the escrow test storage-key fix closing #178. Please align title, summary, and closure reference for accurate tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PR_DESCRIPTION.md` around lines 1 - 8, The PR metadata in PR_DESCRIPTION.md
is inconsistent: the title and Summary mention get_balance and close issue `#74`,
but the actual objective is the escrow test storage-key fix (issue `#178`). Update
the title line, the Summary paragraph text, and the closure reference so they
all describe the escrow test storage-key fix and use "Closes `#178`" (replace any
references to get_balance and "#74"); ensure the summary briefly states the
storage-key test fix and references the escrow contract change to keep tracking
accurate.
Comment on lines
+23
to
+29
| ### � CI/CD Fixes | ||
| - **Branch Naming**: Fixed branch name to follow `feat/` convention | ||
| - **Console.log Removal**: Removed console.log statements from TypeScript test files | ||
| - **Pre-commit Validation**: Ensured all hooks pass validation | ||
|
|
||
| ### �📚 Documentation | ||
| - Complete function documentation with parameter and return value descriptions |
There was a problem hiding this comment.
Fix mojibake characters in section headers.
Line 23 and Line 28 contain replacement characters (�) in headings, which degrades readability in rendered PR docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PR_DESCRIPTION.md` around lines 23 - 29, Replace the replacement characters
in the two PR headings by editing the headings "### � CI/CD Fixes" and "### �📚
Documentation" to valid UTF-8 characters (e.g., use a proper emoji like "⚙️
CI/CD Fixes" and "📚 Documentation" or remove the emoji entirely) and ensure the
file is saved with UTF-8 encoding so the characters render correctly; update
only those heading lines to the corrected text.
Comment on lines
+42
to
+47
| pub fn get_balance(env: Env, commitment: BytesN<32>) -> i128 { | ||
| // Try to read the vault state | ||
| match read_vault(&env, &commitment) { | ||
| Some(vault) => vault.balance, | ||
| None => 0, // Return 0 for non-existent vault (no panic for safe polling) | ||
| } |
There was a problem hiding this comment.
Implementation snippet references the wrong storage reader.
The doc uses read_vault(&env, &commitment), but the storage API shown in gateway-contract/contracts/escrow_contract/src/storage.rs exposes read_vault_state(...) -> Option<VaultState>. This snippet is currently misleading.
Doc snippet correction
-pub fn get_balance(env: Env, commitment: BytesN<32>) -> i128 {
- // Try to read the vault state
- match read_vault(&env, &commitment) {
- Some(vault) => vault.balance,
- None => 0, // Return 0 for non-existent vault (no panic for safe polling)
- }
-}
+pub fn get_balance(env: Env, commitment: BytesN<32>) -> i128 {
+ match read_vault_state(&env, &commitment) {
+ Some(state) => state.balance,
+ None => 0,
+ }
+}📝 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
| pub fn get_balance(env: Env, commitment: BytesN<32>) -> i128 { | |
| // Try to read the vault state | |
| match read_vault(&env, &commitment) { | |
| Some(vault) => vault.balance, | |
| None => 0, // Return 0 for non-existent vault (no panic for safe polling) | |
| } | |
| pub fn get_balance(env: Env, commitment: BytesN<32>) -> i128 { | |
| match read_vault_state(&env, &commitment) { | |
| Some(state) => state.balance, | |
| None => 0, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PR_DESCRIPTION.md` around lines 42 - 47, The doc snippet uses the wrong
storage reader; update get_balance to call the storage function
read_vault_state(...) (which returns Option<VaultState>) instead of read_vault,
and unwrap the returned Option<VaultState> to access .balance or return 0 when
None; reference get_balance and read_vault_state to locate and replace the call
and ensure the types align with VaultState.
Comment on lines
+112
to
117
| ### Modified Files (1) | ||
| - `gateway-contract/contracts/escrow_contract/src/lib.rs` - Added get_balance function | ||
|
|
||
| ### Test Coverage | ||
| - `gateway-contract/contracts/escrow_contract/src/test.rs` - Added 4 comprehensive test cases | ||
|
|
There was a problem hiding this comment.
“Modified Files (1)” conflicts with the file list below it.
Line 112 says one modified file, but Lines 116-120 list additional changed test/CI files. Please make this count consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PR_DESCRIPTION.md` around lines 112 - 117, Update the mismatch in the PR
description by making the "Modified Files (1)" count consistent with the
enumerated file list: either change the header number to reflect the actual
number of modified files (e.g., "Modified Files (4)") or remove the extra file
entries so only the single modified file
(`gateway-contract/contracts/escrow_contract/src/lib.rs`) remains; edit the
PR_DESCRIPTION.md section containing the "Modified Files (1)" header and the
following bullet list to keep the count and the list synchronized.
Comment on lines
83
to
+86
| const hasUsernameLeaf = compileScript.includes('username_leaf_main'); | ||
| console.log(` ${hasUsernameLeaf ? '✅' : '❌'} username_leaf_main in compile list`); | ||
| if (!hasUsernameLeaf) { | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify expected token in verifier script
rg -n "username_leaf_main" zk/verify_implementation.js
# Verify actual circuit names in compile script
rg -n "CIRCUITS|username_leaf_main|username_merkle|username_hash" zk/scripts/compile.shRepository: Alien-Protocol/Alien-Gateway
Length of output: 456
🏁 Script executed:
# Check the context around line 83 in verify_implementation.js
head -n 90 zk/verify_implementation.js | tail -n 15
# Check the full CIRCUITS array in compile.sh
sed -n '17,30p' zk/scripts/compile.sh
# Check if there are any other references to username_leaf_main in compile.sh
grep -n "username_leaf_main" zk/scripts/compile.sh || echo "No matches found"Repository: Alien-Protocol/Alien-Gateway
Length of output: 784
🏁 Script executed:
# View the file to understand the context better
cat -n zk/verify_implementation.js | head -n 100
# Check if username_leaf_main.circom exists
find . -name "username_leaf_main.circom" -type f
# Check what's in the merkle circuits directory
ls -la zk/circuits/merkle/Repository: Alien-Protocol/Alien-Gateway
Length of output: 3567
Add username_leaf_main circuit to compile.sh CIRCUITS array or update the verification check.
The check at line 83 searches for username_leaf_main in compile.sh, but this circuit is not listed in the CIRCUITS array. The circuit file (zk/circuits/merkle/username_leaf_main.circom) exists and is required by the verification script, but it is not compiled by compile.sh. This causes the check at line 85 to fail and hard-fail CI.
Either add "username_leaf_main|merkle/username_leaf_main.circom" to the CIRCUITS array in zk/scripts/compile.sh, or update the verification check in zk/verify_implementation.js if the circuit should not be compiled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/verify_implementation.js` around lines 83 - 86, The verification script is
failing because verify_implementation.js looks for "username_leaf_main" but the
CIRCUITS array in compile.sh does not include that circuit; fix by adding the
entry "username_leaf_main|merkle/username_leaf_main.circom" to the CIRCUITS
array in compile.sh so the circuit is compiled (preferred), or if that circuit
should not be compiled, update the include check in verify_implementation.js to
remove or adjust the "username_leaf_main" presence check so it matches the
actual compiled CIRCUITS array; reference symbols: CIRCUITS array,
"username_leaf_main", compile.sh, verify_implementation.js.
Contributor
|
Hello @olaleyeolajide81-sketch , What are you implemented in this PR . have you see the issue or not . don't toach zk files rollback this change ASAP . |
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
2 participants
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.
#178 [Bug][Contract] Escrow test helper read_vault() reads legacy DataKey::Vault causing storage mismatch
Repo Avatar
Alien-Protocol/Alien-Gateway
📝 Description
In gateway-contract/contracts/escrow_contract/src/test.rs, the read_vault() helper reads from DataKey::Vault(id) (the legacy combined key), but create_vault() writes to the split DataKey::VaultConfig and DataKey::VaultState keys. This means read_vault() in tests will always panic when called after create_vault() in the new architecture.
✅ Requirements
Fix read_vault() in test helpers to read from DataKey::VaultState
Or remove read_vault() and replace usages with direct VaultState reads
Ensure all escrow tests pass with the corrected helper
🎯 Acceptance Criteria
read_vault() reads from the correct storage key
All escrow tests in test.rs pass with cargo test
No panics from missing storage keys
📁 Expected files to change
gateway-contract/contracts/escrow_contract/src/test.rs
💬 Commit: fix(escrow-contract): escrow test helper read_vault() reads legacy Da...
🌿 Branch: fix/escrow-test-read-vault-key | Priority: MEDIUM | Difficulty: ☕ one-coffee
Type: Bug · Priority: MEDIUM · Difficulty: easy · Track: Contract
closes #178
Summary by CodeRabbit
New Features
Chores