Add Ed25519 quote signature verification and integration into SDK#7
Add Ed25519 quote signature verification and integration into SDK#7selfbalance wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 21 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds Ed25519 signature verification for quote responses from the 1Click endpoint. The core verification logic canonicalizes quote data, decodes Base58-encoded keys, and validates signatures using tweetnacl. OneClickService.getQuote() now automatically verifies responses and rejects with QuoteSignatureVerificationError on invalid signatures. Public helper utilities enable consumer re-validation of persisted quotes. Build infrastructure copies the verification module into src/ and wires it into the service. ChangesQuote Signature Verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94bd6182f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| return JSON.stringify(value); | ||
| }; |
There was a problem hiding this comment.
When a base58-encoded Ed25519 value starts with 1 (i.e. the raw public key or signature has a leading zero byte), initializing bytes with [0] and then pushing another zero for each leading 1 decodes one byte too long. In those cases tweetnacl rejects the 65-byte signature/33-byte key and verifyQuoteResponseSignature returns false, so otherwise valid quotes will intermittently fail verification (roughly 1/256 signatures have a leading zero byte).
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (3)
scripts/apply-customizations.cjs (1)
13-29: ⚡ Quick winString-based patching may fail silently if generator output changes.
The exact string matches (e.g., the
return __request(OpenAPI, {pattern and the"Check swap execution status"comment) are tightly coupled to the current generator output. If the OpenAPI generator updates its formatting, imports, or comments, these replacements will silently not apply, leaving the integration broken.Consider adding validation after patching to confirm the expected patterns are present in the final output:
if (!service.includes('withQuoteSignatureVerification(__request(OpenAPI, {')) { throw new Error('Failed to apply withQuoteSignatureVerification wrapper to OneClickService.ts'); }🤖 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/apply-customizations.cjs` around lines 13 - 29, The current string-based patching of servicePath (looking for the exact getQuoteReturn "return __request(OpenAPI, {" and the specific comment block) is brittle and may silently fail if the generator output changes; after performing the replacements involving withQuoteSignatureVerification and the import insertion, validate the modified service string contains the expected marker (e.g., 'withQuoteSignatureVerification(__request(OpenAPI, {') and if not, throw an error explaining the patch failed for servicePath so the CI/dev knows to inspect OneClickService.ts; ensure checks reference the exact symbol withQuoteSignatureVerification and the getQuoteReturn pattern to detect unsuccessful patching.README.md (1)
61-76: ⚡ Quick winShow the new error in a catch example.
This introduces a new
getQuote()failure mode, but the later error-handling section still only demonstratesApiError. A shortQuoteSignatureVerificationErrorexample here would make the behavior much easier for consumers to adopt correctly.🤖 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 `@README.md` around lines 61 - 76, Update the error-handling example to include the new QuoteSignatureVerificationError so consumers can see how to handle signature verification failures alongside ApiError; specifically, in the example that currently catches only ApiError, add a branch (or separate catch) that identifies QuoteSignatureVerificationError (import it alongside ApiError and getQuote()) and shows appropriate handling logic for signature verification failures when calling OneClickService.getQuote(); mention getQuote(), QuoteSignatureVerificationError, and ApiError so reviewers can locate the updated example.scripts/test-generation-and-verification.cjs (1)
149-173: ⚡ Quick winCover missing and malformed signatures explicitly.
The only failing case here is payload tampering. That leaves the missing/malformed signature paths documented in
README.mduntested, even though they exercise different verifier behavior than a hash mismatch. Add at least one case with nosignatureand one with an invalided25519:payload.🤖 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/test-generation-and-verification.cjs` around lines 149 - 173, Add tests to explicitly cover missing and malformed signatures for OneClickService.getQuote: extend the existing axios.request mocks (used in the current test block) to return a response with the quote payload but first without any signature field, and assert that OneClickService.getQuote rejects with QuoteSignatureVerificationError; then add another mock response where signature is present but not prefixed or formatted as a valid "ed25519:" signature (e.g., a wrong prefix or corrupted payload) and assert it also rejects with QuoteSignatureVerificationError; ensure these new cases follow the same pattern as the existing axios.request stubs so they exercise the verifier pathways for missing and malformed signatures.
🤖 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/test-generation-and-verification.cjs`:
- Around line 52-58: The current test uses ts.transpileModule (called with
sourcePath) which skips semantic type-checking; replace that flow with a real
no-emit TypeScript Program: create a ts.Program for the generated source root
(tempRoot/src) including all .ts files, call program.getOptionsDiagnostics() and
program.getSemanticDiagnostics() and fail the test if any diagnostics are
returned, and only then proceed to transpile/write the output to dist; update
the logic around the existing ts.transpileModule usage (and the similar block at
lines 138-141) so semantic/option diagnostics are collected and checked before
emitting or writing files.
In `@src/index.ts`:
- Line 9: The package entrypoint export list in src/index.ts misses re-exporting
withQuoteSignatureVerification so consumers must import it from an internal
path; add withQuoteSignatureVerification to the exported symbols alongside
QuoteSignatureVerificationError, getCanonicalQuoteHash,
getCanonicalQuotePayload, verifyQuoteResponseOrThrow, and
verifyQuoteResponseSignature so the helper is publicly available from the
package root. Locate the export statement that currently re-exports from
'./quoteSignature' and include withQuoteSignatureVerification in that export
list.
In `@src/quoteSignature.ts`:
- Around line 44-73: The decodeBase58 function incorrectly initializes bytes to
[0], which together with the later loop that appends a zero for each leading '1'
produces an extra leading zero; change the initialization of bytes in
decodeBase58 from [0] to an empty array (e.g., const bytes: number[] = []),
leaving the existing main conversion loop and the trailing loop that pushes
zeros for each leading '1' intact so leading-zero base58 characters are
represented by a single 0 byte.
---
Nitpick comments:
In `@README.md`:
- Around line 61-76: Update the error-handling example to include the new
QuoteSignatureVerificationError so consumers can see how to handle signature
verification failures alongside ApiError; specifically, in the example that
currently catches only ApiError, add a branch (or separate catch) that
identifies QuoteSignatureVerificationError (import it alongside ApiError and
getQuote()) and shows appropriate handling logic for signature verification
failures when calling OneClickService.getQuote(); mention getQuote(),
QuoteSignatureVerificationError, and ApiError so reviewers can locate the
updated example.
In `@scripts/apply-customizations.cjs`:
- Around line 13-29: The current string-based patching of servicePath (looking
for the exact getQuoteReturn "return __request(OpenAPI, {" and the specific
comment block) is brittle and may silently fail if the generator output changes;
after performing the replacements involving withQuoteSignatureVerification and
the import insertion, validate the modified service string contains the expected
marker (e.g., 'withQuoteSignatureVerification(__request(OpenAPI, {') and if not,
throw an error explaining the patch failed for servicePath so the CI/dev knows
to inspect OneClickService.ts; ensure checks reference the exact symbol
withQuoteSignatureVerification and the getQuoteReturn pattern to detect
unsuccessful patching.
In `@scripts/test-generation-and-verification.cjs`:
- Around line 149-173: Add tests to explicitly cover missing and malformed
signatures for OneClickService.getQuote: extend the existing axios.request mocks
(used in the current test block) to return a response with the quote payload but
first without any signature field, and assert that OneClickService.getQuote
rejects with QuoteSignatureVerificationError; then add another mock response
where signature is present but not prefixed or formatted as a valid "ed25519:"
signature (e.g., a wrong prefix or corrupted payload) and assert it also rejects
with QuoteSignatureVerificationError; ensure these new cases follow the same
pattern as the existing axios.request stubs so they exercise the verifier
pathways for missing and malformed signatures.
🪄 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: efbc03a6-05c2-4f55-ad96-af2d2737ff05
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
README.mdpackage.jsonscripts/apply-customizations.cjsscripts/templates/quoteSignature.tsscripts/test-generation-and-verification.cjssrc/index.tssrc/quoteSignature.tssrc/services/OneClickService.tstest/fixtures/openapi.quote.yaml
| const output = ts.transpileModule(fs.readFileSync(sourcePath, 'utf8'), { | ||
| compilerOptions: { | ||
| esModuleInterop: true, | ||
| module: ts.ModuleKind.CommonJS, | ||
| target: ts.ScriptTarget.ES2020, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does TypeScript 5.4 transpileModule perform full semantic type-checking, or only syntactic/file-isolated transpilation? Please cite the official TypeScript API documentation.
💡 Result:
TypeScript 5.4 transpileModule does not perform full semantic type-checking; it is intended as a syntactic transpilation API. In particular, its implementation forces isolatedModules and noCheck (meaning it does not run the full type-checker), and it can report only syntactic/options diagnostics rather than semantic type system errors. The official TypeScript API documentation/examples for transpileModule describe it as a “string => string transformation”/shortcut for getting JS output, not a full checking pipeline [1]. The TypeScript compiler API source for transpileModule states it unconditionally enables isolatedModules = true and noCheck = true [2], and its reportDiagnostics path adds only program.getSyntacticDiagnostics(...) plus program.getOptionsDiagnostics() (not program.getSemanticDiagnostics(...)) [2]. So, if you need semantic type-checking (type system errors/warnings), you should create a Program and use semantic diagnostics APIs (e.g., via getSemanticDiagnostics) rather than transpileModule [1][3].
Citations:
- 1: https://github.com/microsoft/TypeScript-wiki/blob/master/Using-the-Compiler-API.md
- 2: https://github.com/Microsoft/TypeScript/blob/master/src/services/transpile.ts
- 3: https://microsoft-typescript.mintlify.app/api/diagnostics
Add a real no-emit TypeScript type-check before transpiling the generated client.
ts.transpileModule() is file-isolated and skips the semantic type-checker (it effectively runs in noCheck/isolatedModules mode and reports only syntactic/options diagnostics), so this E2E can stay green even if the generated SDK has TypeScript semantic type errors. Create a ts.Program for tempRoot/src and fail the test on semantic/option diagnostics before writing dist/.
Also applies to: 138-141
🤖 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/test-generation-and-verification.cjs` around lines 52 - 58, The
current test uses ts.transpileModule (called with sourcePath) which skips
semantic type-checking; replace that flow with a real no-emit TypeScript
Program: create a ts.Program for the generated source root (tempRoot/src)
including all .ts files, call program.getOptionsDiagnostics() and
program.getSemanticDiagnostics() and fail the test if any diagnostics are
returned, and only then proceed to transpile/write the output to dist; update
the logic around the existing ts.transpileModule usage (and the similar block at
lines 138-141) so semantic/option diagnostics are collected and checked before
emitting or writing files.
| export { CancelablePromise, CancelError } from './core/CancelablePromise'; | ||
| export { OpenAPI } from './core/OpenAPI'; | ||
| export type { OpenAPIConfig } from './core/OpenAPI'; | ||
| export { QuoteSignatureVerificationError, getCanonicalQuoteHash, getCanonicalQuotePayload, verifyQuoteResponseOrThrow, verifyQuoteResponseSignature } from './quoteSignature'; |
There was a problem hiding this comment.
Re-export withQuoteSignatureVerification from the package entrypoint.
Line 9 exposes the other new verification helpers but omits withQuoteSignatureVerification, so part of the new API is only reachable via an internal module path.
Suggested fix
-export { QuoteSignatureVerificationError, getCanonicalQuoteHash, getCanonicalQuotePayload, verifyQuoteResponseOrThrow, verifyQuoteResponseSignature } from './quoteSignature';
+export {
+ QuoteSignatureVerificationError,
+ getCanonicalQuoteHash,
+ getCanonicalQuotePayload,
+ verifyQuoteResponseOrThrow,
+ verifyQuoteResponseSignature,
+ withQuoteSignatureVerification,
+} from './quoteSignature';📝 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.
| export { QuoteSignatureVerificationError, getCanonicalQuoteHash, getCanonicalQuotePayload, verifyQuoteResponseOrThrow, verifyQuoteResponseSignature } from './quoteSignature'; | |
| export { | |
| QuoteSignatureVerificationError, | |
| getCanonicalQuoteHash, | |
| getCanonicalQuotePayload, | |
| verifyQuoteResponseOrThrow, | |
| verifyQuoteResponseSignature, | |
| withQuoteSignatureVerification, | |
| } from './quoteSignature'; |
🤖 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 `@src/index.ts` at line 9, The package entrypoint export list in src/index.ts
misses re-exporting withQuoteSignatureVerification so consumers must import it
from an internal path; add withQuoteSignatureVerification to the exported
symbols alongside QuoteSignatureVerificationError, getCanonicalQuoteHash,
getCanonicalQuotePayload, verifyQuoteResponseOrThrow, and
verifyQuoteResponseSignature so the helper is publicly available from the
package root. Locate the export statement that currently re-exports from
'./quoteSignature' and include withQuoteSignatureVerification in that export
list.
| const decodeBase58 = (value: string): Uint8Array => { | ||
| const bytes = [0]; | ||
|
|
||
| for (const char of value) { | ||
| const index = BASE58_ALPHABET.indexOf(char); | ||
| if (index === -1) { | ||
| throw new Error(`Invalid base58 character: ${char}`); | ||
| } | ||
|
|
||
| let carry = index; | ||
| for (let i = 0; i < bytes.length; i += 1) { | ||
| carry += bytes[i] * 58; | ||
| bytes[i] = carry & 0xff; | ||
| carry >>= 8; | ||
| } | ||
|
|
||
| while (carry > 0) { | ||
| bytes.push(carry & 0xff); | ||
| carry >>= 8; | ||
| } | ||
| } | ||
|
|
||
| for (const char of value) { | ||
| if (char !== '1') { | ||
| break; | ||
| } | ||
| bytes.push(0); | ||
| } | ||
|
|
||
| return new Uint8Array(bytes.reverse()); |
There was a problem hiding this comment.
Fix the base58 decoder's leading-zero handling.
Line 45 starts bytes at [0], and Lines 66-71 then append another zero for every leading '1'. That makes values like '1' decode to two zero bytes instead of one, so valid signatures/public keys with a leading zero byte will be rejected.
Suggested fix
- for (const char of value) {
- if (char !== '1') {
- break;
- }
- bytes.push(0);
- }
+ for (let i = 0; i < value.length - 1 && value[i] === '1'; i += 1) {
+ bytes.push(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.
| const decodeBase58 = (value: string): Uint8Array => { | |
| const bytes = [0]; | |
| for (const char of value) { | |
| const index = BASE58_ALPHABET.indexOf(char); | |
| if (index === -1) { | |
| throw new Error(`Invalid base58 character: ${char}`); | |
| } | |
| let carry = index; | |
| for (let i = 0; i < bytes.length; i += 1) { | |
| carry += bytes[i] * 58; | |
| bytes[i] = carry & 0xff; | |
| carry >>= 8; | |
| } | |
| while (carry > 0) { | |
| bytes.push(carry & 0xff); | |
| carry >>= 8; | |
| } | |
| } | |
| for (const char of value) { | |
| if (char !== '1') { | |
| break; | |
| } | |
| bytes.push(0); | |
| } | |
| return new Uint8Array(bytes.reverse()); | |
| const decodeBase58 = (value: string): Uint8Array => { | |
| const bytes = [0]; | |
| for (const char of value) { | |
| const index = BASE58_ALPHABET.indexOf(char); | |
| if (index === -1) { | |
| throw new Error(`Invalid base58 character: ${char}`); | |
| } | |
| let carry = index; | |
| for (let i = 0; i < bytes.length; i += 1) { | |
| carry += bytes[i] * 58; | |
| bytes[i] = carry & 0xff; | |
| carry >>= 8; | |
| } | |
| while (carry > 0) { | |
| bytes.push(carry & 0xff); | |
| carry >>= 8; | |
| } | |
| } | |
| for (let i = 0; i < value.length - 1 && value[i] === '1'; i += 1) { | |
| bytes.push(0); | |
| } | |
| return new Uint8Array(bytes.reverse()); |
🤖 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 `@src/quoteSignature.ts` around lines 44 - 73, The decodeBase58 function
incorrectly initializes bytes to [0], which together with the later loop that
appends a zero for each leading '1' produces an extra leading zero; change the
initialization of bytes in decodeBase58 from [0] to an empty array (e.g., const
bytes: number[] = []), leaving the existing main conversion loop and the
trailing loop that pushes zeros for each leading '1' intact so leading-zero
base58 characters are represented by a single 0 byte.
1 participant
Motivation
getQuoteautomatically enforces signature checks as defense-in-depth.Description
src/quoteSignature.tsimplementing canonical payload/hash, base58 decoding, Ed25519 verification usingjs-sha256andtweetnacl, and helper exportsverifyQuoteResponseSignature,verifyQuoteResponseOrThrow,getCanonicalQuotePayload,getCanonicalQuoteHash, andwithQuoteSignatureVerification.OneClickService.getQuoteto callwithQuoteSignatureVerification(...)so quote responses are verified before resolving, and export verification helpers fromsrc/index.ts.scripts/apply-customizations.cjsand templatescripts/templates/quoteSignature.tsto inject verification into generated clients, plus a fixturetest/fixtures/openapi.quote.yamlto exercise generation.scripts/test-generation-and-verification.cjsand a newtest:e2escript inpackage.json, and add runtime dependenciesjs-sha256andtweetnaclwith corresponding lockfile updates.README.mdwith aQuote Signature Verificationsection documenting automatic verification and exported helpers.Testing
scripts/test-generation-and-verification.cjsthat generates a customized client, injects a test keypair, transpiles the generated code, and asserts successful verification and failure on tampering.pnpm run test:e2e(which runspnpm buildandnode scripts/test-generation-and-verification.cjs), and the test passed.Codex Task