feat(zk-sdk): add StellarTxBuilder for Soroban register and resolve f…#229
Open
Hassan-oladipupo wants to merge 1 commit intoAlien-Protocol:mainfrom
Open
feat(zk-sdk): add StellarTxBuilder for Soroban register and resolve f…#229Hassan-oladipupo wants to merge 1 commit intoAlien-Protocol:mainfrom
Hassan-oladipupo wants to merge 1 commit intoAlien-Protocol:mainfrom
Conversation
|
@Hassan-oladipupo 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! 🚀 |
📝 WalkthroughWalkthroughThis PR introduces a new ZK SDK package implementing a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StellarTxBuilder
participant SorobanRpc as Soroban RPC Server
participant Contract
Client->>StellarTxBuilder: buildRegister(params)
StellarTxBuilder->>SorobanRpc: Fetch source account
SorobanRpc-->>StellarTxBuilder: Account data
StellarTxBuilder->>StellarTxBuilder: Convert inputs to ScVal
StellarTxBuilder->>StellarTxBuilder: Build transaction
StellarTxBuilder->>SorobanRpc: prepareTransaction(tx)
SorobanRpc-->>StellarTxBuilder: Prepared XDR + metadata
StellarTxBuilder-->>Client: BuiltTransaction {xdr, transaction, method, source}
Client->>StellarTxBuilder: submitTransaction(built, signer)
StellarTxBuilder->>StellarTxBuilder: Sign with Keypair
StellarTxBuilder->>SorobanRpc: sendTransaction(signedXdr)
SorobanRpc->>Contract: Invoke contract method
Contract-->>SorobanRpc: Transaction result
SorobanRpc-->>StellarTxBuilder: Response
StellarTxBuilder-->>Client: Submission result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (4)
zk/sdk/src/__tests__/tx.test.ts (1)
35-44: Missing test coverage forbuildRegisterResolver.The test exercises
buildRegister,buildAddStellarAddress, andbuildResolve, but does not coverbuildRegisterResolver. Consider adding a test case for this method to ensure complete coverage of the public API.🧪 Proposed addition for buildRegisterResolver test
const commitment = new Uint8Array(32).fill(7); + const proof = new Uint8Array(128).fill(1); // Example proof bytes + const publicSignals = { + oldRoot: new Uint8Array(32).fill(2), + newRoot: new Uint8Array(32).fill(3), + }; const registerTx = await builder.buildRegister({ caller: source, commitment, }); + const registerResolverTx = await builder.buildRegisterResolver({ + caller: source, + commitment, + proof, + publicSignals, + }); const addAddressTx = await builder.buildAddStellarAddress({Then include
registerResolverTxin the validation loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/src/__tests__/tx.test.ts` around lines 35 - 44, Add a test that calls builder.buildRegisterResolver(...) and assigns the result to registerResolverTx, then include registerResolverTx in the same validation/assertion loop used for registerTx/addAddressTx/resolveTx. Specifically, invoke the buildRegisterResolver method on the same builder (e.g., builder.buildRegisterResolver with the appropriate caller and commitment/usernameHash arguments), store it as registerResolverTx, and add it to the array or series of transactions being validated so the test covers buildRegisterResolver alongside buildRegister, buildAddStellarAddress, and buildResolve.zk/sdk/package.json (1)
5-6: Consider pointingmainandtypesto compiled output for better compatibility.For a TypeScript package, pointing
mainandtypesdirectly to.tssource files works only when consumers use ts-node or similar. If this SDK may be consumed by projects without TypeScript compilation at import time, consider building todist/and updating these fields accordingly. Since this is marked"private": true, this is acceptable for internal monorepo use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/package.json` around lines 5 - 6, The package.json currently sets "main": "src/index.ts" and "types": "src/index.ts", which points consumers to TypeScript sources; change these to the compiled outputs (e.g., "main": "dist/index.js" and "types": "dist/index.d.ts"), ensure the build step outputs into dist (adjust the build script and tsconfig.outDir if needed), and confirm that the compiled artifacts are produced before publishing or being consumed; look for the "main" and "types" entries in package.json and the build scripts/tsconfig to implement this.zk/sdk/src/types.ts (1)
27-30: Note:PublicSignalsInputrequires both roots from caller.Per the circuit at
zk/circuits/merkle/merkle_update_proof.circom,publicSignalsfrom snarkjs only contains the single outputout_newRoot. TheoldRootmust be provided separately (typically the current on-chain SMT root). This interface correctly requires callers to supply both values for the contract'sPublicSignalsstruct. Consider adding a JSDoc comment to clarify this for SDK consumers.📝 Proposed documentation
+/** + * Public signals for the Groth16 non-inclusion proof. + * Note: `oldRoot` should be the current on-chain SMT root (fetched from contract state). + * `newRoot` is the expected root after registration (from proof generation). + */ export interface PublicSignalsInput { oldRoot: Bytes32Input; newRoot: Bytes32Input; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/src/types.ts` around lines 27 - 30, The PublicSignalsInput interface requires callers to supply both oldRoot and newRoot, but it's unclear because the circom circuit (merkle_update_proof.circom) and snarkjs publicSignals only emit out_newRoot; update the SDK by adding a JSDoc comment above the PublicSignalsInput interface explaining that publicSignals from snarkjs contains only out_newRoot and that oldRoot must be provided separately (e.g., the current on-chain SMT root) when constructing the contract's PublicSignals struct; reference PublicSignalsInput, publicSignals, and out_newRoot in the comment so consumers know where the values originate.zk/sdk/src/tx.ts (1)
148-159: Edge case: base64 strings starting with valid hex prefix.If a base64-encoded value happens to start with characters that look like
0xfollowed by valid hex (e.g.,0xABC...which is valid base64), the function will incorrectly decode it as hex. While rare, consider requiring an explicit format parameter or documenting the precedence behavior.💡 Alternative: explicit format or different detection
-function normalizeBytes(value: string | Uint8Array): Buffer { +function normalizeBytes(value: string | Uint8Array, format?: "hex" | "base64"): Buffer { if (typeof value !== "string") { return Buffer.from(value); } + if (format === "hex") { + const normalized = value.startsWith("0x") ? value.slice(2) : value; + return Buffer.from(normalized, "hex"); + } + if (format === "base64") { + return Buffer.from(value, "base64"); + } + + // Auto-detect: 0x prefix indicates hex const normalized = value.startsWith("0x") ? value.slice(2) : value; if (normalized.length % 2 === 0 && /^[0-9a-fA-F]+$/.test(normalized)) { return Buffer.from(normalized, "hex"); } return Buffer.from(value, "base64"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@zk/sdk/src/tx.ts` around lines 148 - 159, The normalizeBytes function can mis-detect ambiguous inputs (e.g., strings like "0xABC..." that are both valid hex and valid base64); add an optional parameter to normalizeBytes such as format?: 'hex' | 'base64' | 'auto' (default 'auto') and change the logic so that in 'auto' mode you still detect hex/base64 but if an input is ambiguous (passes the hex regex and could be base64) you throw an explicit error asking the caller to supply format, and when format is provided enforce that decoding branch; update callers or document that callers must pass format to disambiguate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@zk/sdk/src/tx.ts`:
- Around line 70-83: The submitTransaction implementation ignores the
SubmitTransactionOptions.pollUntilSuccess flag; update submitTransaction (and
respect _options.pollUntilSuccess) so that after signing and calling
this.server.sendTransaction you extract the returned transaction hash/id and,
when pollUntilSuccess is true, poll the network (using existing server methods
such as a transaction status endpoint like getTransaction/getTx or
waitForTransaction) until the transaction reaches a success or final failure
state, then return the final confirmed result; if pollUntilSuccess is false,
keep the current behavior and return the immediate sendTransaction response.
Ensure you reference submitTransaction, SubmitTransactionOptions,
pollUntilSuccess, and this.server.sendTransaction when making the changes.
---
Nitpick comments:
In `@zk/sdk/package.json`:
- Around line 5-6: The package.json currently sets "main": "src/index.ts" and
"types": "src/index.ts", which points consumers to TypeScript sources; change
these to the compiled outputs (e.g., "main": "dist/index.js" and "types":
"dist/index.d.ts"), ensure the build step outputs into dist (adjust the build
script and tsconfig.outDir if needed), and confirm that the compiled artifacts
are produced before publishing or being consumed; look for the "main" and
"types" entries in package.json and the build scripts/tsconfig to implement
this.
In `@zk/sdk/src/__tests__/tx.test.ts`:
- Around line 35-44: Add a test that calls builder.buildRegisterResolver(...)
and assigns the result to registerResolverTx, then include registerResolverTx in
the same validation/assertion loop used for registerTx/addAddressTx/resolveTx.
Specifically, invoke the buildRegisterResolver method on the same builder (e.g.,
builder.buildRegisterResolver with the appropriate caller and
commitment/usernameHash arguments), store it as registerResolverTx, and add it
to the array or series of transactions being validated so the test covers
buildRegisterResolver alongside buildRegister, buildAddStellarAddress, and
buildResolve.
In `@zk/sdk/src/tx.ts`:
- Around line 148-159: The normalizeBytes function can mis-detect ambiguous
inputs (e.g., strings like "0xABC..." that are both valid hex and valid base64);
add an optional parameter to normalizeBytes such as format?: 'hex' | 'base64' |
'auto' (default 'auto') and change the logic so that in 'auto' mode you still
detect hex/base64 but if an input is ambiguous (passes the hex regex and could
be base64) you throw an explicit error asking the caller to supply format, and
when format is provided enforce that decoding branch; update callers or document
that callers must pass format to disambiguate.
In `@zk/sdk/src/types.ts`:
- Around line 27-30: The PublicSignalsInput interface requires callers to supply
both oldRoot and newRoot, but it's unclear because the circom circuit
(merkle_update_proof.circom) and snarkjs publicSignals only emit out_newRoot;
update the SDK by adding a JSDoc comment above the PublicSignalsInput interface
explaining that publicSignals from snarkjs contains only out_newRoot and that
oldRoot must be provided separately (e.g., the current on-chain SMT root) when
constructing the contract's PublicSignals struct; reference PublicSignalsInput,
publicSignals, and out_newRoot in the comment so consumers know where the values
originate.
🪄 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: f9911cde-d193-43a7-ba94-3eefdd8ffb1e
⛔ Files ignored due to path filters (1)
zk/sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
zk/sdk/package.jsonzk/sdk/src/__tests__/tx.test.tszk/sdk/src/index.tszk/sdk/src/tx.tszk/sdk/src/types.ts
Comment on lines
+70
to
+83
| public async submitTransaction( | ||
| built: BuiltTransaction | string, | ||
| signer: Keypair | string, | ||
| _options: SubmitTransactionOptions = {}, | ||
| ) { | ||
| const signed = typeof built === "string" | ||
| ? TransactionBuilder.fromXDR(built, this.config.networkPassphrase) | ||
| : TransactionBuilder.fromXDR(built.xdr, this.config.networkPassphrase); | ||
| const keypair = typeof signer === "string" ? Keypair.fromSecret(signer) : signer; | ||
|
|
||
| signed.sign(keypair); | ||
|
|
||
| return this.server.sendTransaction(signed); | ||
| } |
There was a problem hiding this comment.
SubmitTransactionOptions.pollUntilSuccess is declared but not used.
The _options parameter includes pollUntilSuccess (per types.ts), but the implementation ignores it and returns immediately after sendTransaction. This could leave callers expecting confirmation polling that never happens.
🔧 Proposed implementation for pollUntilSuccess
public async submitTransaction(
built: BuiltTransaction | string,
signer: Keypair | string,
- _options: SubmitTransactionOptions = {},
+ options: SubmitTransactionOptions = {},
) {
const signed = typeof built === "string"
? TransactionBuilder.fromXDR(built, this.config.networkPassphrase)
: TransactionBuilder.fromXDR(built.xdr, this.config.networkPassphrase);
const keypair = typeof signer === "string" ? Keypair.fromSecret(signer) : signer;
signed.sign(keypair);
- return this.server.sendTransaction(signed);
+ const response = await this.server.sendTransaction(signed);
+
+ if (options.pollUntilSuccess && response.status === "PENDING") {
+ // Poll for transaction result
+ const hash = response.hash;
+ let result = await this.server.getTransaction(hash);
+ while (result.status === "NOT_FOUND") {
+ await new Promise((resolve) => setTimeout(resolve, 1000));
+ result = await this.server.getTransaction(hash);
+ }
+ return result;
+ }
+
+ return response;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@zk/sdk/src/tx.ts` around lines 70 - 83, The submitTransaction implementation
ignores the SubmitTransactionOptions.pollUntilSuccess flag; update
submitTransaction (and respect _options.pollUntilSuccess) so that after signing
and calling this.server.sendTransaction you extract the returned transaction
hash/id and, when pollUntilSuccess is true, poll the network (using existing
server methods such as a transaction status endpoint like getTransaction/getTx
or waitForTransaction) until the transaction reaches a success or final failure
state, then return the final confirmed result; if pollUntilSuccess is false,
keep the current behavior and return the immediate sendTransaction response.
Ensure you reference submitTransaction, SubmitTransactionOptions,
pollUntilSuccess, and this.server.sendTransaction when making the changes.
ryzen-xp
added
Approved
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.
feat(zk-sdk): add StellarTxBuilder for Soroban register and resolve flows
close #198
Summary by CodeRabbit
New Features
Tests