feat: release 1.1.6 with stellar contract validation support - #18
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
AI review: 0 P0 / 3 P1 / 3 P2 — no blocking issues foundLabeled P0 (blocking)None. P1 (should fix)
P2 (nice to have)
ai-pr-review · claude-sonnet-4-6 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10ba31fc5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| export function isValidStellarAddress(address: string): boolean { | ||
| return StrKey.isValidEd25519PublicKey(address); | ||
| return StrKey.isValidEd25519PublicKey(address) || StrKey.isValidContract(address); |
There was a problem hiding this comment.
Do not label contract addresses as payments
When parseDeeplink receives a valid C-address, it invokes parseAddress before parseStellar; this expanded validator therefore enters the Stellar branch in handlers/address.ts, which unconditionally sets operation: "pay". A Soroban contract address is not an account payment destination, so consumers that dispatch on operation can incorrectly start a Stellar payment flow for a scanned contract. Distinguish accounts from contracts in this path and omit or use a contract-specific operation for C-addresses, matching the operation-free result returned by parseStellar itself.
Useful? React with 👍 / 👎.
AI review: 0 P0 / 3 P1 / 3 P2 — no blocking issues foundLabeled P0 (blocking)None. P1 (should fix)
P2 (nice to have)
ai-pr-review · claude-sonnet-4-6 |
AI review: 0 P0 / 4 P1 / 4 P2 — no blocking issues foundLabeled P0 (blocking)None. P1 (should fix)
P2 (nice to have)
ai-pr-review · claude-sonnet-4-6 |
shawnmuggle
left a comment
There was a problem hiding this comment.
Two issues should be resolved before merge:
-
P1 — bump the release to 1.1.7+: version 1.1.6 is already published on npm from commit b10ba31, while this PR head is 1a39836. Any follow-up fix cannot overwrite the published 1.1.6 artifacts. Please bump all three package versions and internal dependency pins, then regenerate the lockfile.
-
P2 — do not classify Stellar contract IDs as account payments:
isValidStellarAddress()now accepts C-addresses, soparseAddress()returnsoperation: "pay"for a Soroban contract. Consumers dispatching onoperationcan incorrectly start an account-payment flow. Please distinguish G- and C-addresses here and add a regression assertion that a C-address does not receiveoperation: "pay".
I verified the current head: 50/50 tests pass, build and type-check pass, and CI/Vercel are green. The blockers above are semantic/release issues not covered by those checks.
AI review: 0 P0 / 5 P1 / 4 P2 — no blocking issues foundLabeled P0 (blocking)None. P1 (should fix)
P2 (nice to have)
ai-pr-review · claude-sonnet-4-6 |
There was a problem hiding this comment.
Automated code review — P0=0 P1=1 P2=3
Stellar C-address (Soroban contract) parsing support plus version bumps to 1.1.7; main risk is that isValidStellarAddress now accepts contract addresses, so SEP-7 pay destinations and other consumers may treat a non-payable contract as a payment account.
- P1
packages/core/src/protocols/stellar.ts:45 — isValidStellarAddress() now returns true for C-addresses. Any consumer (including SEP-7 pay destination validation and downstream apps like rozo-invoice) that used this helper to gate a payment flow will accept a Soroban contract address as a payment destination, sending an account payment to a non-account address — funds can be irrecoverably lost. The PR's own docs state contract addresses must not start a payment flow, but this widening contradicts that. Fix: Keep isValidStellarAddress() as account-only (Ed25519) and add a separate isValidStellarContract()/isValidStellarAddressOrContract() helper; use the explicit contract check only in parseAddress/parseStellar plain-address handling. - P2
packages/core/src/protocols/stellar.ts:28 — parseStellar() returns the same shape for G and C addresses, so callers cannot distinguish an account from a contract from the result alone; a wallet auto-starting a pay flow on any parsed stellar address will do so for contracts. Fix: Add a discriminating field (e.g. address_type: "account" | "contract") to StellarParseResult and set it in both parseStellar and parseAddress, and document it in the README type block. - P2
packages/core/src/tests/address.test.ts:32 — expect(result).not.toHaveProperty("operation", "pay") passes even if operation is absent for the wrong reason, and no test asserts chain_id is omitted for contracts; a regression re-adding operation/chain_id under a different value would go undetected. Fix: Assert explicitly that operation and chain_id are undefined for C-addresses, and add a negative test that a C-address is rejected as a SEP-7 pay destination. - P2
packages/core/package.json:23 — Version jumps 1.1.4 -> 1.1.7 while the PR title says 1.1.6, and the lockfile already resolves published 1.1.7 tarballs for the workspace packages — the demo app may resolve the registry copy rather than the workspace source, so the reviewed code is not what is tested. Fix: Reconcile the title/version, and use workspace:* for internal deps in apps/demo and packages so local source is exercised.
Reviewer: claude-opus-5
Approved for this exact head SHA; the PR author decides when to merge.
No description provided.