Payments 4/7: Custodial Wallet Onboarding & Non-Custodial Upgrade Path - #1580
Merged
yusuftomilola merged 2 commits intoAug 22, 2026
Merged
Conversation
Ships the wallet_accounts model with two custody paths: - Custodial: KeyCustodyService generates a Stellar keypair server-side and envelope-encrypts the secret at rest (AES-256-GCM data key, wrapped by a swappable KeyManagementService abstraction). No other module ever sees a decrypted key; every decrypt is audited in wallet_key_access_log. Provisioning is idempotent under concurrent requests via the same DB-unique-constraint pattern as PaymentsService. - Non-custodial: single-use nonce challenge-response linking, verified against the claimed Stellar public key. Linking an external wallet while a custodial one exists upgrades that account in place. A minimal ledger (wallet_ledger_entries) gives custodial wallets a store-credit-style balance via an admin funding stub — real on-chain transfer is out of scope per the issue. Frontend ships a wallet status card with the balance framed as store credit and the raw address tucked behind an "Advanced" disclosure. Closes DistinctCodes#1573
|
@AbdulmujibOladayo is attempting to deploy a commit to the naijabuz's projects Team on Vercel. A member of the Team first needs to authorize it. |
- key-custody.service.spec.ts: mock EntityManager didn't structurally satisfy provisionKeypair's EntityManager parameter type; type the mock factory's return as any (it's a test double, not a real one). - key-management.service.spec.ts: makeService's default parameter (`= MASTER_KEY`) was silently substituted even when the "no master key configured" test explicitly passed `undefined` — JS applies a default parameter on any undefined argument, explicit or not. That let a real gap through: the test asserted a clean failure but was actually exercising the configured-master-key path. Drop the default and require every call site to pass its key explicitly.
yusuftomilola
approved these changes
Aug 22, 2026
yusuftomilola
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the custodial wallet onboarding & non-custodial upgrade path PR (closes #1573).
- The key custody model is well-layered:
KeyManagementServicewraps a data key with a master key (envelope encryption), onlyKeyCustodyServiceever touches a decrypted secret, and every decrypt is recorded inwallet_key_access_logwith actor/reason. That access-log requirement in particular is exactly what you want for a system holding user funds — it turns "who decrypted this key and why" from an unanswerable question into an auditable one. The abstraction being swappable for a real KMS later without touching callers is the right way to ship an MVP custodial store without painting the design into a corner. - Confirmed no endpoint, DTO, or log line exposes the secret — only the public
walletAddress— and this is backed by actual tests (persisted material never contains the plaintext secret,failed decrypt logs a failed access entry and throws cleanly with no secret in the error message), not just a description. - Idempotent provisioning via the DB-unique-constraint-as-source-of-truth pattern — generate the keypair only after the
wallet_accountsinsert wins the(user_id)constraint — correctly mirrors the pattern already established inPaymentsService#initiate, so a double-click/race can't provision two keypairs for one user. Good consistency with the existing codebase convention rather than inventing a new one. - The challenge-response linking flow locking the challenge row for the duration of check-and-consume closes the replay window at the DB layer, and rejecting a captured/replayed signature plus expired-challenge plus already-claimed-address are all explicitly tested rather than assumed.
- Upgrading
CUSTODIAL → EXTERNALin place when a user links an external wallet (instead of creating a duplicate wallet record) is the right UX/data model — a user shouldn't end up with two wallet identities because they later decided to bring their own. - Tamper-tag rejection on the envelope encryption round-trip being explicitly tested is a meaningful check for GCM-based encryption specifically — that's the test that would catch a broken authentication-tag verification if one were ever introduced.
- The funding stub being clearly scoped as a store-credit ledger entry, not a real on-chain transfer, and the frontend explicitly avoiding "seed phrase" language and tucking the raw address behind an "Advanced" disclosure, both show the custodial-UX tradeoffs were made deliberately.
CI is green across Backend, Frontend, and Frontend E2E (the PR's own checklist listed CI as pending at time of writing, but it has since completed successfully). Vercel's FAILURE is the usual unauthorized deployment integration link, unrelated to the code.
Approving — careful, well-audited handling of a genuinely high-stakes piece (custodial key storage).
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
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.
Summary
Implements custodial Stellar wallet provisioning and non-custodial wallet linking, per the issue's scope.
WalletsService.provisionCustodialWalletgenerates a Stellar keypair server-side. The secret is envelope-encrypted at rest (AES-256-GCM data key, wrapped by aKeyManagementServiceabstraction backed today by a local AES-256-GCM master key, swappable for a real KMS later without touching callers). OnlyKeyCustodyServiceever touches a decrypted key; every decrypt is recorded inwallet_key_access_logwith actor/reason. No endpoint, DTO, or log line ever exposes the secret — only the publicwalletAddress.PaymentsService#initiate(Payments 1/7: Payment Domain Model, Initiation Flow & Idempotent Transaction Lifecycle #1570) — a keypair is only ever generated after thewallet_accountsinsert wins the(user_id)unique constraint, so a double-click/race can never provision two keypairs for one user.POST /wallets/link/challengeissues a single-use, short-lived nonce;POST /wallets/link/verifyverifies a signature over that nonce against the claimed Stellar public key. The challenge row is locked for the duration of the check-and-consume so a captured signature can't be replayed. Linking an external wallet when the user already has a custodial one upgrades that account in place (CUSTODIAL → EXTERNAL) rather than creating a duplicate; linking an address already claimed by another account is rejected via a partial unique index.POST /wallets/:userId/fundrecords awallet_ledger_entriescredit — a store-credit-style balance, not a real on-chain transfer (explicitly out of scope per the issue).WalletStatusCardcomponent (/walletpage) framed as a store-credit balance, with the raw address tucked behind an "Advanced" disclosure and a "connect a wallet you already own" flow — no onboarding copy uses the phrase "seed phrase".See
backend/src/wallets/README.mdfor the module's design notes.Test plan
WalletsService: idempotent provisioning under a simulated concurrent-insert race, admin funding validation (non-custodial/inactive/positive-amount/reason-required), external-link happy path + upgrade path + replay rejection + expiry rejection + invalid-signature rejection + duplicate-address rejection + idempotent re-link.KeyCustodyService: provisioned address is a valid Stellar public key, persisted material never contains the plaintext secret,sign()round-trips a verifiable signature, failed decrypt logs a failed access entry and throws cleanly with no secret in the error message.EnvelopeKeyManagementService: wrap/unwrap round-trip, tampered-tag rejection, clean failure with no master key configured.Closes #1573