diff --git a/pkg/api/handler_wallets_create.go b/pkg/api/handler_wallets_create.go index 06d1d6b..1d9d95b 100644 --- a/pkg/api/handler_wallets_create.go +++ b/pkg/api/handler_wallets_create.go @@ -3,6 +3,7 @@ package api import ( "net/http" + "github.com/formancehq/go-libs/v5/pkg/transport/api" wallet "github.com/formancehq/wallets/pkg" "github.com/go-chi/render" ) @@ -16,7 +17,7 @@ func (m *MainHandler) createWalletHandler(w http.ResponseWriter, r *http.Request } } - wallet, err := m.manager.CreateWallet(r.Context(), data) + wallet, err := m.manager.CreateWallet(r.Context(), api.IdempotencyKeyFromRequest(r), data) if err != nil { internalError(w, r, err) return diff --git a/pkg/api/handler_wallets_create_test.go b/pkg/api/handler_wallets_create_test.go index f656c07..4a3175c 100644 --- a/pkg/api/handler_wallets_create_test.go +++ b/pkg/api/handler_wallets_create_test.go @@ -51,3 +51,47 @@ func TestWalletsCreate(t *testing.T) { require.Equal(t, wallet.Metadata, createWalletRequest.Metadata) require.Equal(t, wallet.Name, createWalletRequest.Name) } + +func TestWalletsCreateIdempotency(t *testing.T) { + t.Parallel() + + const idempotencyKey = "create-wallet-key-1" + + // A real idempotent retry replays the *same* request body, so the payload + // is fixed across both calls (a regenerated name would be a different body + // with the same key, which the ledger treats as a conflict, not a replay). + request := wallet.CreateRequest{Name: "savings-account"} + + var ( + forwardedKeys []string + targetedAccounts []string + ) + testEnv := newTestEnv( + WithAddMetadataToAccount(func(ctx context.Context, l, a, ik string, m map[string]string) error { + forwardedKeys = append(forwardedKeys, ik) + targetedAccounts = append(targetedAccounts, a) + return nil + }), + ) + + create := func() *wallet.Wallet { + req := newRequest(t, http.MethodPost, "/wallets", request) + req.Header.Set("Idempotency-Key", idempotencyKey) + rec := httptest.NewRecorder() + testEnv.Router().ServeHTTP(rec, req) + require.Equal(t, http.StatusCreated, rec.Result().StatusCode) + w := &wallet.Wallet{} + readResponse(t, rec, w) + return w + } + + first := create() + second := create() + + // The Idempotency-Key is forwarded to the ledger and the derived wallet ID + // (hence the targeted account) is stable across retries, so the retry hits + // the same account instead of creating a duplicate wallet. + require.Equal(t, []string{idempotencyKey, idempotencyKey}, forwardedKeys) + require.Equal(t, first.ID, second.ID) + require.Equal(t, targetedAccounts[0], targetedAccounts[1]) +} diff --git a/pkg/api/handler_wallets_debit.go b/pkg/api/handler_wallets_debit.go index d6c4bba..c125f60 100644 --- a/pkg/api/handler_wallets_debit.go +++ b/pkg/api/handler_wallets_debit.go @@ -32,6 +32,8 @@ func (m *MainHandler) debitWalletHandler(w http.ResponseWriter, r *http.Request) wallet.IsErrInvalidAccountName(err), wallet.IsErrInvalidAsset(err): badRequest(w, ErrorCodeValidation, wallet.ErrInvalidBalanceSpecified) + case errors.Is(err, wallet.ErrNonIdempotentDebit): + badRequest(w, ErrorCodeValidation, wallet.ErrNonIdempotentDebit) default: internalError(w, r, err) } diff --git a/pkg/api/handler_wallets_debit_test.go b/pkg/api/handler_wallets_debit_test.go index 5abaf76..5267ff0 100644 --- a/pkg/api/handler_wallets_debit_test.go +++ b/pkg/api/handler_wallets_debit_test.go @@ -268,63 +268,65 @@ func TestWalletsDebit(t *testing.T) { var ( testEnv *testEnv + chart *wallet.Chart + ledgerName string postTransaction wallet.PostTransaction ) testEnv = newTestEnv( WithGetAccount(func(ctx context.Context, ledger, account string) (*wallet.AccountWithVolumesAndBalances, error) { switch account { - case testEnv.Chart().GetMainBalanceAccount(walletID): + case chart.GetMainBalanceAccount(walletID): return &wallet.AccountWithVolumesAndBalances{ Account: wallet.Account{ - Address: testEnv.Chart().GetMainBalanceAccount(walletID), + Address: chart.GetMainBalanceAccount(walletID), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "main", }.LedgerMetadata(walletID)), }, }, nil - case testEnv.Chart().GetBalanceAccount(walletID, "coupon1"): + case chart.GetBalanceAccount(walletID, "coupon1"): return &wallet.AccountWithVolumesAndBalances{ Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "coupon1"), + Address: chart.GetBalanceAccount(walletID, "coupon1"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "coupon1", ExpiresAt: ptr(time.Now().Add(5 * time.Second)), }.LedgerMetadata(walletID)), }, }, nil - case testEnv.Chart().GetBalanceAccount(walletID, "coupon2"): + case chart.GetBalanceAccount(walletID, "coupon2"): return &wallet.AccountWithVolumesAndBalances{ Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "coupon2"), + Address: chart.GetBalanceAccount(walletID, "coupon2"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "coupon2", Priority: 10, }.LedgerMetadata(walletID)), }, }, nil - case testEnv.Chart().GetBalanceAccount(walletID, "coupon3"): + case chart.GetBalanceAccount(walletID, "coupon3"): return &wallet.AccountWithVolumesAndBalances{ Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "coupon3"), + Address: chart.GetBalanceAccount(walletID, "coupon3"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "coupon3", ExpiresAt: ptr(time.Now().Add(-time.Minute)), }.LedgerMetadata(walletID)), }, }, nil - case testEnv.Chart().GetBalanceAccount(walletID, "coupon4"): + case chart.GetBalanceAccount(walletID, "coupon4"): return &wallet.AccountWithVolumesAndBalances{ Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "coupon4"), + Address: chart.GetBalanceAccount(walletID, "coupon4"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "coupon4", }.LedgerMetadata(walletID)), }, }, nil - case testEnv.Chart().GetBalanceAccount(walletID, "secondary"): + case chart.GetBalanceAccount(walletID, "secondary"): return &wallet.AccountWithVolumesAndBalances{ Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "secondary"), + Address: chart.GetBalanceAccount(walletID, "secondary"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "secondary", }.LedgerMetadata(walletID)), @@ -335,14 +337,14 @@ func TestWalletsDebit(t *testing.T) { } }), WithListAccounts(func(ctx context.Context, ledger string, query wallet.ListAccountsQuery) (*wallet.AccountsCursorResponseCursor, error) { - require.Equal(t, testEnv.LedgerName(), ledger) + require.Equal(t, ledgerName, ledger) require.Equal(t, query.Metadata, wallet.BalancesMetadataFilter(walletID)) return &wallet.AccountsCursorResponseCursor{ Data: []wallet.AccountWithVolumesAndBalances{ { Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "coupon2"), + Address: chart.GetBalanceAccount(walletID, "coupon2"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "coupon2", Priority: 10, @@ -351,7 +353,7 @@ func TestWalletsDebit(t *testing.T) { }, { Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "coupon1"), + Address: chart.GetBalanceAccount(walletID, "coupon1"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "coupon1", ExpiresAt: ptr(time.Now().Add(5 * time.Second)), @@ -360,7 +362,7 @@ func TestWalletsDebit(t *testing.T) { }, { Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "coupon3"), + Address: chart.GetBalanceAccount(walletID, "coupon3"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "coupon3", ExpiresAt: ptr(time.Now().Add(-time.Minute)), @@ -369,7 +371,7 @@ func TestWalletsDebit(t *testing.T) { }, { Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "coupon4"), + Address: chart.GetBalanceAccount(walletID, "coupon4"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "coupon4", }.LedgerMetadata(walletID)), @@ -377,7 +379,7 @@ func TestWalletsDebit(t *testing.T) { }, { Account: wallet.Account{ - Address: testEnv.Chart().GetBalanceAccount(walletID, "main"), + Address: chart.GetBalanceAccount(walletID, "main"), Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ Name: "main", }.LedgerMetadata(walletID)), @@ -387,7 +389,7 @@ func TestWalletsDebit(t *testing.T) { }, nil }), WithCreateTransaction(func(ctx context.Context, ledger, ik string, p wallet.PostTransaction) (*shared.V2Transaction, error) { - require.Equal(t, testEnv.LedgerName(), ledger) + require.Equal(t, ledgerName, ledger) postTransaction = p if testCase.postTransactionError != nil { return nil, testCase.postTransactionError @@ -396,6 +398,8 @@ func TestWalletsDebit(t *testing.T) { return nil, nil }), ) + chart = testEnv.Chart() + ledgerName = testEnv.LedgerName() testEnv.Router().ServeHTTP(rec, req) expectedStatusCode := testCase.expectedStatusCode @@ -428,3 +432,120 @@ func TestWalletsDebit(t *testing.T) { }) } } + +func TestWalletsDebitPendingIdempotency(t *testing.T) { + t.Parallel() + + const idempotencyKey = "debit-pending-key-1" + walletID := uuid.NewString() + + var bodies []wallet.PostTransaction + testEnv := newTestEnv( + WithCreateTransaction(func(ctx context.Context, ledger, ik string, p wallet.PostTransaction) (*shared.V2Transaction, error) { + require.Equal(t, idempotencyKey, ik) + bodies = append(bodies, p) + //nolint:nilnil + return nil, nil + }), + ) + + debit := func() *wallet.DebitHold { + req := newRequest(t, http.MethodPost, "/wallets/"+walletID+"/debit", wallet.DebitRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Pending: true, + }) + req.Header.Set("Idempotency-Key", idempotencyKey) + rec := httptest.NewRecorder() + testEnv.Router().ServeHTTP(rec, req) + require.Equal(t, http.StatusCreated, rec.Result().StatusCode) + hold := &wallet.DebitHold{} + readResponse(t, rec, hold) + return hold + } + + // With a stable (explicit, non-expiring) source set, retrying a pending + // debit under the same Idempotency-Key yields the same hold ID *and* a + // byte-identical ledger request body — so the ledger sees a genuine replay, + // not a conflict, and no duplicate hold is created. + require.Equal(t, debit().ID, debit().ID) + require.Len(t, bodies, 2) + require.Equal(t, bodies[0], bodies[1]) +} + +func TestWalletsDebitWithIdempotencyKeyRejectsNonReplayableSources(t *testing.T) { + t.Parallel() + + // A debit body resolved from live ledger state cannot be replayed + // byte-for-byte: an expiring balance can cross its expiry boundary and a + // wildcard set can change between attempts, so the ledger (which hashes the + // body to enforce idempotency) would reject the retry as a conflict. We + // therefore refuse such debits up front when an Idempotency-Key is present + // rather than offer a false idempotency guarantee. + for _, tc := range []struct { + name string + balances []string + }{ + {name: "expiring balance", balances: []string{"promo"}}, + {name: "wildcard balance", balances: []string{"*"}}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + const idempotencyKey = "debit-non-replayable-key-1" + walletID := uuid.NewString() + + var ( + chart *wallet.Chart + created bool + ) + testEnv := newTestEnv( + WithGetAccount(func(ctx context.Context, ledger, account string) (*wallet.AccountWithVolumesAndBalances, error) { + return &wallet.AccountWithVolumesAndBalances{ + Account: wallet.Account{ + Address: account, + Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ + Name: "promo", + ExpiresAt: ptr(time.Now().Add(time.Hour)), + }.LedgerMetadata(walletID)), + }, + }, nil + }), + WithListAccounts(func(ctx context.Context, ledger string, query wallet.ListAccountsQuery) (*wallet.AccountsCursorResponseCursor, error) { + return &wallet.AccountsCursorResponseCursor{ + Data: []wallet.AccountWithVolumesAndBalances{ + { + Account: wallet.Account{ + Address: chart.GetBalanceAccount(walletID, "main"), + Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ + Name: "main", + }.LedgerMetadata(walletID)), + }, + }, + }, + }, nil + }), + WithCreateTransaction(func(ctx context.Context, ledger, ik string, p wallet.PostTransaction) (*shared.V2Transaction, error) { + created = true + //nolint:nilnil + return nil, nil + }), + ) + chart = testEnv.Chart() + + req := newRequest(t, http.MethodPost, "/wallets/"+walletID+"/debit", wallet.DebitRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Pending: true, + Balances: tc.balances, + }) + req.Header.Set("Idempotency-Key", idempotencyKey) + rec := httptest.NewRecorder() + testEnv.Router().ServeHTTP(rec, req) + + require.Equal(t, http.StatusBadRequest, rec.Result().StatusCode) + errorResponse := readErrorResponse(t, rec) + require.Equal(t, ErrorCodeValidation, errorResponse.ErrorCode) + require.False(t, created, "no transaction should be submitted to the ledger") + }) + } +} diff --git a/pkg/error.go b/pkg/error.go index 0a658bb..187211c 100644 --- a/pkg/error.go +++ b/pkg/error.go @@ -18,6 +18,13 @@ var ( ErrInvalidBalanceSpecified = errors.New("invalid balance specified") ErrNegativeAmount = errors.New("negative amount provided") ErrValidation = errors.New("validation error") + // ErrNonIdempotentDebit is returned when a debit carries an Idempotency-Key + // but its source set cannot be resolved deterministically (wildcard sources + // or sources with an expiry). The ledger enforces idempotency by hashing the + // whole request body, so such a debit could not be safely replayed: a retry + // might resolve to a different body and be rejected as a conflict. We reject + // it up front rather than offer a false idempotency guarantee. + ErrNonIdempotentDebit = errors.New("debit cannot be made idempotent: wildcard or expiring balances are not allowed with an idempotency key") ) type GenericOpenAPIError interface { diff --git a/pkg/manager.go b/pkg/manager.go index d0c1bac..af96619 100644 --- a/pkg/manager.go +++ b/pkg/manager.go @@ -12,9 +12,24 @@ import ( "github.com/formancehq/formance-sdk-go/v3/pkg/models/shared" "github.com/formancehq/go-libs/v5/pkg/types/metadata" + "github.com/google/uuid" "github.com/pkg/errors" ) +// idempotencyNamespace seeds deterministic resource IDs derived from an +// Idempotency-Key, so that retrying a creation request resolves to the same +// wallet/hold rather than creating a duplicate. +var idempotencyNamespace = uuid.MustParse("0b6f2d6e-4e2a-4f3a-9f0a-2b9c1d8e7a31") + +// deterministicID derives a stable UUID from an Idempotency-Key, scoped by a +// resource kind ("wallet", "hold", ...). The kind discriminator keeps the +// derived IDs of different resource types disjoint, so reusing the same +// Idempotency-Key across, say, a wallet creation and a pending debit cannot +// collide on the same UUID. +func deterministicID(kind, ik string) string { + return uuid.NewSHA1(idempotencyNamespace, []byte(kind+":"+ik)).String() +} + type ListResponse[T any] struct { Data []T Next, Previous string @@ -124,6 +139,12 @@ func (m *Manager) Debit(ctx context.Context, ik string, debit Debit) (*DebitHold } hold = Ptr(debit.newHold()) + // Derive the hold ID from the Idempotency-Key so a retry produces an + // identical ledger request (the ledger hashes the body to enforce + // idempotency) and returns the same hold. + if ik != "" { + hold.ID = deterministicID("hold", ik) + } holdAccount := m.chart.GetHoldAccount(hold.ID) metadata = map[string]map[string]string{ holdAccount: hold.LedgerMetadata(m.chart), @@ -142,6 +163,12 @@ func (m *Manager) Debit(ctx context.Context, ik string, debit Debit) (*DebitHold Name: MainBalance, }} case len(debit.Balances) == 1 && debit.Balances[0] == "*": + // A wildcard source set is resolved from live ledger state, so it can + // differ between two attempts. We cannot guarantee an identical ledger + // body on retry, so refuse to pretend the call is idempotent. + if ik != "" { + return nil, ErrNonIdempotentDebit + } balances, err = fetchAndMapAllAccounts[Balance](ctx, m, BalancesMetadataFilter(debit.WalletID), false, BalanceFromAccount) if err != nil { return nil, err @@ -164,8 +191,17 @@ func (m *Manager) Debit(ctx context.Context, ik string, debit Debit) (*DebitHold var sources []string // Filter expired and generate sources for _, balance := range balances { - if balance.ExpiresAt != nil && !balance.ExpiresAt.IsZero() && balance.ExpiresAt.Before(time.Now()) { - continue + if balance.ExpiresAt != nil && !balance.ExpiresAt.IsZero() { + if balance.ExpiresAt.Before(time.Now()) { + continue + } + // The balance is live now but will expire: crossing that boundary + // between two attempts would drop it from the source set and change + // the ledger body. We cannot honour idempotency in that case, so + // reject rather than offer a false guarantee. + if ik != "" { + return nil, ErrNonIdempotentDebit + } } sources = append(sources, m.chart.GetBalanceAccount(debit.WalletID, balance.Name)) } @@ -433,14 +469,19 @@ func (m *Manager) ListTransactions(ctx context.Context, query ListQuery[ListTran }), nil } -func (m *Manager) CreateWallet(ctx context.Context, data *CreateRequest) (*Wallet, error) { +func (m *Manager) CreateWallet(ctx context.Context, ik string, data *CreateRequest) (*Wallet, error) { wallet := NewWallet(data.Name, m.ledgerName, data.Metadata) + // Derive the wallet ID from the Idempotency-Key so a retry targets the + // same account instead of creating a duplicate wallet. + if ik != "" { + wallet.ID = deterministicID("wallet", ik) + } if err := m.client.AddMetadataToAccount( ctx, m.ledgerName, m.chart.GetMainBalanceAccount(wallet.ID), - "", + ik, wallet.LedgerMetadata(), ); err != nil { return nil, errors.Wrap(err, "adding metadata to account")