Skip to content

fix(backend): move default wallet creation out of middleware - #755

Open
DarianM wants to merge 3 commits into
mainfrom
darian/wal-1145-3
Open

fix(backend): move default wallet creation out of middleware#755
DarianM wants to merge 3 commits into
mainfrom
darian/wal-1145-3

Conversation

@DarianM

@DarianM DarianM commented Aug 4, 2026

Copy link
Copy Markdown
Member

Context

Closes WAL-1145

Some users have two wallets, both named default
(... GROUP BY user_id HAVING COUNT(*) > 1 returns duplicates).

Root cause

The default wallet was created lazily inside the HTTP and gRPC middleware, on every authenticated request.
On a fresh user's first burst of parallel requests, more than one could pass the "no wallet yet" check and each insert a wallet.

Changes proposed in this pull request

  • No schema change, no migration
  • Moved wallet creation to a single point: it now happens in CompleteSignup
    (grpc/signup.go), right after Kratos registration
    • off the per-request path, so the race that caused the bug can't happen there
    • it runs before anything needs the wallet (dashboard, KYC, naming).
  • Removed wallet creation from the middleware
    • they only resolve the existing wallet into context now, and tolerate its absence (a user can be authenticated before CompleteSignup runs).
  • Kept the existing advisory lock in ops.Create fix: pg advisory lock to prevent double wallet create race condition #326 as the concurrency guard: it serializes
    concurrent/retried CompleteSignup calls per user (first creates, rest reuse), so a user
    doesn't end up with two.

Copilot AI lite review requested due to automatic review settings August 4, 2026 13:01
@linear

linear Bot commented Aug 4, 2026

Copy link
Copy Markdown
WAL-1145 Wallets get created twice: once with default and then with name

When a wallet is created with default if the user leaves the app the wallet is created again when the user rejoins.

Expecte:

User uses the same wallet

Actual:

A new default wallet is created

If there is already a wallet attached to the user do not create a new wallet.

Use the following queries to reproduce:

SELECT wallet_id
FROM  user_wallets t
WHERE t.user_id IN (
  SELECT user_id
  FROM user_wallets
  GROUP BY user_id
  HAVING COUNT(*) > 1
);

select * from wallets where id in (SELECT wallet_id
FROM  user_wallets t
WHERE t.user_id IN (
  SELECT user_id
  FROM user_wallets
  GROUP BY user_id
  HAVING COUNT(*) > 1
)
)

Update: 10.07.2026

  • Remove user_wallets constraint to have only one user-wallet relationship
  • Add cleanup task to clean up the duplicate default wallets

Updates 14.07.2026

Root cause

The default wallet was created lazily (as a side effect) inside the HTTP and gRPC middleware, on every authenticated request ("if the user has no wallet, create one").
When a fresh user's first page load fired several requests at once, more than one could pass the "no wallet yet" check and each insert a wallet.
A prior fix (interledger/interledger-app#326) guarded this with an advisory lock, but the database never enforced uniqueness, so nothing structurally prevented a second wallet.

Review in Linear

@DarianM
DarianM temporarily deployed to e2e-scaler-system August 4, 2026 13:01 — with GitHub Actions Inactive
@github-actions github-actions Bot added documentation Improvements or additions to documentation backend labels Aug 4, 2026
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

E2E Test Report

E2E markdown report is available as an artifact:

This comment was marked as resolved.

@DarianM
DarianM temporarily deployed to e2e-scaler-system August 4, 2026 13:50 — with GitHub Actions Inactive
@DarianM
DarianM requested a lite review from Copilot August 4, 2026 13:50
@DarianM
DarianM deployed to e2e-tester August 4, 2026 14:02 — with GitHub Actions Active

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (4)

go/backend/grpc/user.go:14

  • Leaving a production TODO that claims the handler is unused is not actionable and can easily become stale/misleading. If CreateUserDefaultWallet is truly obsolete after moving wallet creation into CompleteSignup, consider removing the handler (and any service registration / proto exposure) or replacing the TODO with a deprecation comment that includes an issue/ticket and removal timeline.
//TODO: unused handler, nothing calls it.
func (s *rpcService) CreateUserDefaultWallet(ctx context.Context, req *pb.CreateUserDefaultWalletRequest) (*pb.Empty, error) {

go/backend/grpc/signup.go:93

  • The error message "signup already completed" is ambiguous for legitimate retries by the same user vs. a conflicting user binding. Consider making the message explicit about the conflict (e.g., that the signup is already associated with a different user) to reduce support/debug time.
	if su.UserID != "" && su.UserID != req.UserId {
		return nil, ForbiddenError("signup already completed")
	}

documentation/docs/signup-guide.md:588

  • Signup().Complete itself is a fallible step and is executed after the wallet is created, so it’s not accurate to say everything that can fail happens before finalization. Suggest rewording to something like: "all steps that should abort the request (signup ownership check + wallet creation) run before Signup().Complete; after completion, only best-effort work remains". This keeps the guide aligned with the actual control flow and failure modes.
**Timing:** Inside `CompleteSignup`, before the signup is marked complete. The handler reads the signup, rejects it if another user already owns it, creates the wallet, and only then calls `Signup().Complete` — so everything that can fail the request happens before the signup is finalized (and before its "new signup" notification fires).

go/backend/grpc/signup_test.go:180

  • This test validates the new CompleteSignup behavior but does not enforce call order. Since the PR’s correctness relies on doing Get + wallet Create before SignupService.Complete, consider using gomock.InOrder(...) (as in TestCompleteSignup_NoAgreementSigning) here (and in similar tests below) to prevent regressions that would still satisfy unordered expectations.
	c.SignupService.EXPECT().Complete(gomock.Any(), sID, userID).Return(nil).Times(1)
	c.SignupService.EXPECT().Get(gomock.Any(), sID).Return(&signup.Signup{CountryCode: "US"}, nil).Times(1)
	c.walletImpl.EXPECT().Create(gomock.Any(), wallets.CreateArgs{UserID: userID, Country: country.US}).Return(nil, nil).Times(1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants