feat: Add Idempotency-Hit header #1155
Conversation
gfyrag
commented
Nov 24, 2025
gfyrag
commented
- feat: return idempotency hit from controller
- feat: add Idempotency-Hit header
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughController methods gained an idempotencyHit boolean return; handlers inspect it and set an Idempotency-Hit response header when true. Mocks were regenerated to typed per-call wrappers and tests updated. Client models/docs expose response Headers and some requests accept Idempotency-Key. New e2e tests verify header behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as API Handler
participant Controller
participant LogProcessor as Log Processor
Client->>Handler: HTTP request (may include Idempotency-Key)
Handler->>Controller: call Create/Revert/Save/Delete(...)
Controller->>LogProcessor: forgeLog / find by idempotency key
alt first request
LogProcessor-->>Controller: (log, output, false, nil)
else idempotent replay
LogProcessor-->>Controller: (log, output, true, nil)
end
Controller-->>Handler: (log, output, idempotencyHit, err)
alt idempotencyHit == true
Handler->>Handler: set header "Idempotency-Hit: true"
end
Handler-->>Client: HTTP response (body + Headers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/v2/controllers_transactions_create_test.go (1)
390-440: CreateTransaction mock returns are correct, but the test still captures the loop variable improperly.The updated expectations:
- success:
expect.Return(&ledger.Log{}, &ledger.CreatedTransaction{Transaction: expectedTx}, false, nil)- error:
expect.Return(nil, nil, false, tc.returnError)correctly match the new
(*ledger.Log, *ledger.CreatedTransaction, bool, error)signature.However, inside the loop you still mix
testCaseandtcinside the subtest closure, which means several fields read from the range variable instead of the copied value and can end up using the last test’s data. It’s safer to always usetcinside the closure.for _, testCase := range testCases { - tc := testCase - t.Run(tc.name, func(t *testing.T) { - if testCase.expectedStatusCode == 0 { - testCase.expectedStatusCode = http.StatusOK - } + tc := testCase + t.Run(tc.name, func(t *testing.T) { + if tc.expectedStatusCode == 0 { + tc.expectedStatusCode = http.StatusOK } ... - systemController, ledgerController := newTestingSystemController(t, true) - if testCase.expectControllerCall { - testCase.expectedRunScript.Timestamp = time.Time{} + systemController, ledgerController := newTestingSystemController(t, true) + if tc.expectControllerCall { + tc.expectedRunScript.Timestamp = time.Time{} expect := ledgerController.EXPECT(). CreateTransaction(gomock.Any(), ledgercontroller.Parameters[ledgercontroller.CreateTransaction]{ - DryRun: tc.expectedDryRun, + DryRun: tc.expectedDryRun, Input: ledgercontroller.CreateTransaction{ - RunScript: testCase.expectedRunScript, + RunScript: tc.expectedRunScript, }, }) ... - require.Equal(t, testCase.expectedStatusCode, rec.Code) - if testCase.expectedStatusCode < 300 && testCase.expectedStatusCode >= 200 { + require.Equal(t, tc.expectedStatusCode, rec.Code) + if tc.expectedStatusCode < 300 && tc.expectedStatusCode >= 200 { ... - } else { + } else { ... - require.EqualValues(t, testCase.expectedErrorCode, err.ErrorCode) + require.EqualValues(t, tc.expectedErrorCode, err.ErrorCode) } }) }
🧹 Nitpick comments (14)
pkg/client/models/operations/v2createtransaction.go (1)
62-88: Headers exposure looks good; consider tightening nil-handling semanticsThe added
Headersfield andGetHeadersaccessor are consistent with the rest of the response model and give callers straightforward access to HTTP headers; nil-safe handling on the receiver is good.If you want more predictable semantics for consumers (never having to special‑case nil vs empty maps), you could also treat a nil
Headersfield as empty:func (o *V2CreateTransactionResponse) GetHeaders() map[string][]string { - if o == nil { - return map[string][]string{} - } - return o.Headers + if o == nil || o.Headers == nil { + return map[string][]string{} + } + return o.Headers }Not strictly necessary, but it avoids the subtle difference between
o == nilando.Headers == niland may simplify caller code.pkg/client/models/operations/v2reverttransaction.go (1)
87-92: Consider returningnilinGetHeadersfor a nil receiverRight now a nil receiver returns a fresh empty map. That’s safe, but slightly surprising because:
- Callers cannot distinguish “no response” from “no headers”.
- Any mutation on the returned map (when receiver is nil) won’t persist anywhere.
If you prefer clearer semantics, you could return
nilinstead:func (o *V2RevertTransactionResponse) GetHeaders() map[string][]string { - if o == nil { - return map[string][]string{} - } + if o == nil { + return nil + } return o.Headers }Not critical, just a minor clarity tweak (would need to be done in the generator).
Also applies to: 108-113
internal/controller/ledger/controller_with_too_many_client_handling_test.go (1)
31-41: Tests correctly updated to the 4‑tuple CreateTransaction signatureThe expectations and call sites now match
CreateTransaction’s(log, created, bool, error)shape, and the retry behavior is still asserted via the error path. If you later add tests around the idempotency flag itself, it may be worth adding an assertion on the returned bool, but that’s optional for this “too many clients” behavior.Also applies to: 51-52, 65-68, 79-80
pkg/client/docs/models/operations/v2reverttransactionrequest.md (1)
13-13: IdempotencyKey field doc looks correct; consider clarifying usage.The new
IdempotencyKeyfield is consistent with the existing table structure and typing. If you want to be extra explicit, you could mention the exact HTTP header name (e.g. “Sent via theIdempotency-Keyheader”) so callers immediately see how to supply it.internal/api/v1/controllers_transactions_create.go (1)
87-90: Both CreateTransaction call sites are correctly adapted to the new signature.Using
_, res, _, err := l.CreateTransaction(...)in both the postings and script branches aligns with the new 4‑value return and preserves existing v1 behavior. If you later decide to exposeIdempotency-Hiton v1 as well, this is where you’d start wiring the boolean through to response headers.Also applies to: 118-125
internal/api/v2/controllers_accounts_delete_metadata_test.go (1)
73-73: Mock signature correctly updated.The mock expectation now returns three values
(*ledger.Log, bool, error)to match the updatedDeleteAccountMetadatainterface.Consider adding test cases to verify the
Idempotency-Hitheader is set correctly whenidempotencyHitis true:{ name: "idempotency hit", expectBackendCall: true, account: "account0", returnIdempotencyHit: true, // Add this field to testCase },Then verify in the test:
if tc.returnIdempotencyHit { require.Equal(t, "true", rec.Header().Get("Idempotency-Hit")) }internal/api/v1/controllers_transactions_add_metadata.go (1)
33-44: SaveTransactionMetadata call correctly adjusted; consider idempotency header for v1The call site is correctly updated for the new
(*ledger.Log, bool, error)signature, and existing error handling is preserved.If the intent is for v1 endpoints to also surface idempotent replays, you may want to capture the boolean and optionally set an
Idempotency-Hitresponse header here (mirroring the v2 behavior); if v1 is not supposed to expose this, the current implementation is fine.internal/api/v2/controllers_transactions_delete_metadata_test.go (1)
69-89: Add Idempotency-Hit header assertions in the happy path.This suite now depends on the handler surfacing
Idempotency-Hit, but none of the cases assert it. Please add a success case withidempotencyHit = trueand verify the response header so a future regression is caught.@@ type testCase struct { name string queryParams url.Values returnErr error expectedStatusCode int expectedErrorCode string expectBackendCall bool + idempotencyHit bool } @@ { name: "nominal", expectBackendCall: true, }, + { + name: "idempotency hit", + expectBackendCall: true, + idempotencyHit: true, + }, @@ - Return(&ledger.Log{}, false, tc.returnErr) + Return(&ledger.Log{}, tc.idempotencyHit, tc.returnErr) @@ router.ServeHTTP(rec, req) if tc.expectedStatusCode == 0 || tc.expectedStatusCode == http.StatusOK { require.Equal(t, http.StatusNoContent, rec.Code) } else { require.Equal(t, tc.expectedStatusCode, rec.Code) errorResponse := api.ErrorResponse{} require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &errorResponse)) require.Equal(t, tc.expectedErrorCode, errorResponse.ErrorCode) } + + header := rec.Header().Get("Idempotency-Hit") + if rec.Code == http.StatusNoContent && tc.idempotencyHit { + require.Equal(t, "true", header) + } else { + require.Empty(t, header) + }internal/api/v2/controllers_transactions_add_metadata_test.go (1)
91-115: Assert Idempotency-Hit in the metadata POST tests.We expose the
Idempotency-Hitheader from this handler now, but the suite never checks it. Add a case where the mock returnsidempotencyHit = trueand assert the header to guarantee the new contract.@@ type testCase struct { name string queryParams url.Values expectStatusCode int expectedErrorCode string body any id any expectBackendCall bool returnErr error + idempotencyHit bool } @@ { name: "nominal", body: metadata.Metadata{ "foo": "bar", }, expectBackendCall: true, }, + { + name: "idempotency hit", + body: metadata.Metadata{ + "foo": "bar", + }, + expectBackendCall: true, + idempotencyHit: true, + }, @@ - Return(nil, false, testCase.returnErr) + Return(nil, testCase.idempotencyHit, testCase.returnErr) @@ router.ServeHTTP(rec, req) require.Equal(t, testCase.expectStatusCode, rec.Code) if testCase.expectStatusCode >= 300 || testCase.expectStatusCode < 200 { err := api.ErrorResponse{} api.Decode(t, rec.Body, &err) require.EqualValues(t, testCase.expectedErrorCode, err.ErrorCode) } + + header := rec.Header().Get("Idempotency-Hit") + if rec.Code == http.StatusNoContent && testCase.idempotencyHit { + require.Equal(t, "true", header) + } else { + require.Empty(t, header) + }test/e2e/api_idempotency_hit_header_test.go (1)
1-477: Comprehensive Idempotency-Hit e2e coverage; only minor possible DRY refactorsThe scenarios exercise all main write operations with/without prior calls and correctly assert that
Idempotency-Hitis absent on first use and"true"on replay with the same idempotency key, which aligns well with the intended semantics. If you want to reduce repetition later, you could factor small helpers for building common requests and asserting the header value, but that's purely optional given this file’s clarity.internal/controller/ledger/controller_with_events.go (1)
43-154: Idempotency flag propagation looks correct; consider event emission on idempotent replaysThe wrappers correctly pass through
idempotencyHitfrom the underlying controller and force it tofalseon error, preserving existing behavior while extending the signature.One thing to double‑check: all six methods (
CreateTransaction,RevertTransaction,Save*Metadata,Delete*Metadata) still schedule listener events whenever!parameters.DryRun, even whenidempotencyHit == true. That means repeated idempotent calls will re‑emitCommittedTransactions/SavedMetadata/DeletedMetadataevents to listeners.If the desired contract is “no additional side‑effects on idempotent replays”, you may want to gate event emission on
!idempotencyHitas well:if !parameters.DryRun && !idempotencyHit { c.handleEvent(ctx, func() { // existing listener call }) }If, instead, duplicate notifications are acceptable (or relied upon), current behavior is fine but should probably be documented.
internal/controller/ledger/controller_with_traces.go (1)
365-411: Traced Create/Revert correctly capture and propagate idempotencyHit
CreateTransactionandRevertTransactionnow recordidempotencyHitinside theTraceWithMetricclosure and return(nil, nil, false, err)on failure,(log, payload, idempotencyHit, nil)on success. UsingRET = anypurely as a vehicle for tracing (and discarding the value with_) is fine here.If you want to tighten things stylistically, you could mirror the metadata methods and use a more specific
RETtype instead ofany, but functionally this is sound.internal/controller/ledger/controller.go (1)
19-75: Documentation for boolean return values recommendedVerification confirms all six methods and their implementations have been correctly updated with the new
boolreturn type across all Controller implementations (DefaultController,ControllerWithEvents,ControllerWithTraces,ControllerWithTooManyClientHandling, and the generated mock). No signature mismatches found.The signature changes are complete and consistent. For clarity, consider adding brief doc comments explaining what the boolean represents (e.g., idempotency flag, whether state changed) on these six methods in the interface:
CreateTransactionRevertTransactionSaveTransactionMetadataSaveAccountMetadataDeleteTransactionMetadataDeleteAccountMetadatainternal/controller/ledger/controller_with_too_many_client_handling.go (1)
18-18: Clean up duplicate-typedflag in go:generateThe go:generate line includes
-typedtwice, once as a flag and once trailing afterDelayCalculator. That extra token is unnecessary and may be confusing for future maintainers or tooling.Consider simplifying to a single flag:
-//go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed +//go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.logpkg/client/speakeasyusagegen/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.log
📒 Files selected for processing (66)
docs/api/README.md(7 hunks)internal/README.md(1 hunks)internal/api/bulking/bulker.go(6 hunks)internal/api/bulking/bulker_test.go(13 hunks)internal/api/bulking/mocks.go(1 hunks)internal/api/bulking/mocks_ledger_controller_test.go(20 hunks)internal/api/common/mocks_ledger_controller_test.go(6 hunks)internal/api/v1/controllers_accounts_add_metadata.go(1 hunks)internal/api/v1/controllers_accounts_add_metadata_test.go(1 hunks)internal/api/v1/controllers_accounts_delete_metadata.go(1 hunks)internal/api/v1/controllers_accounts_delete_metadata_test.go(1 hunks)internal/api/v1/controllers_transactions_add_metadata.go(1 hunks)internal/api/v1/controllers_transactions_add_metadata_test.go(1 hunks)internal/api/v1/controllers_transactions_create.go(2 hunks)internal/api/v1/controllers_transactions_create_test.go(1 hunks)internal/api/v1/controllers_transactions_delete_metadata.go(1 hunks)internal/api/v1/controllers_transactions_delete_metadata_test.go(1 hunks)internal/api/v1/controllers_transactions_revert.go(1 hunks)internal/api/v1/controllers_transactions_revert_test.go(1 hunks)internal/api/v1/mocks.go(1 hunks)internal/api/v1/mocks_ledger_controller_test.go(20 hunks)internal/api/v1/mocks_system_controller_test.go(33 hunks)internal/api/v2/controllers_accounts_add_metadata.go(1 hunks)internal/api/v2/controllers_accounts_add_metadata_test.go(1 hunks)internal/api/v2/controllers_accounts_delete_metadata.go(1 hunks)internal/api/v2/controllers_accounts_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_bulk_test.go(13 hunks)internal/api/v2/controllers_transactions_add_metadata.go(2 hunks)internal/api/v2/controllers_transactions_add_metadata_test.go(1 hunks)internal/api/v2/controllers_transactions_create.go(2 hunks)internal/api/v2/controllers_transactions_create_test.go(1 hunks)internal/api/v2/controllers_transactions_delete_metadata.go(1 hunks)internal/api/v2/controllers_transactions_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_transactions_revert.go(2 hunks)internal/api/v2/controllers_transactions_revert_test.go(1 hunks)internal/api/v2/mocks.go(1 hunks)internal/api/v2/mocks_ledger_controller_test.go(20 hunks)internal/api/v2/mocks_system_controller_test.go(33 hunks)internal/controller/ledger/controller.go(2 hunks)internal/controller/ledger/controller_default.go(6 hunks)internal/controller/ledger/controller_default_test.go(4 hunks)internal/controller/ledger/controller_generated_test.go(20 hunks)internal/controller/ledger/controller_with_events.go(6 hunks)internal/controller/ledger/controller_with_too_many_client_handling.go(2 hunks)internal/controller/ledger/controller_with_too_many_client_handling_generated_test.go(2 hunks)internal/controller/ledger/controller_with_too_many_client_handling_test.go(4 hunks)internal/controller/ledger/controller_with_traces.go(1 hunks)internal/controller/ledger/log_process.go(2 hunks)internal/controller/ledger/log_process_test.go(2 hunks)internal/controller/system/state_tracker.go(1 hunks)internal/log.go(1 hunks)pkg/client/docs/models/operations/v2addmetadataontransactionresponse.md(1 hunks)pkg/client/docs/models/operations/v2addmetadatatoaccountresponse.md(1 hunks)pkg/client/docs/models/operations/v2createtransactionresponse.md(1 hunks)pkg/client/docs/models/operations/v2deleteaccountmetadataresponse.md(1 hunks)pkg/client/docs/models/operations/v2deletetransactionmetadataresponse.md(1 hunks)pkg/client/docs/models/operations/v2reverttransactionrequest.md(1 hunks)pkg/client/docs/models/operations/v2reverttransactionresponse.md(1 hunks)pkg/client/models/operations/v2addmetadataontransaction.go(2 hunks)pkg/client/models/operations/v2addmetadatatoaccount.go(2 hunks)pkg/client/models/operations/v2createtransaction.go(2 hunks)pkg/client/models/operations/v2deleteaccountmetadata.go(2 hunks)pkg/client/models/operations/v2deletetransactionmetadata.go(2 hunks)pkg/client/models/operations/v2reverttransaction.go(4 hunks)pkg/client/v2.go(7 hunks)test/e2e/api_idempotency_hit_header_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (30)
internal/api/v2/controllers_accounts_add_metadata_test.go (1)
internal/log.go (1)
Log(84-96)
internal/api/bulking/bulker.go (2)
internal/controller/ledger/controller.go (6)
CreateTransaction(96-100)SaveAccountMetadata(114-117)SaveTransactionMetadata(109-112)RevertTransaction(102-107)DeleteAccountMetadata(124-127)DeleteTransactionMetadata(119-122)internal/controller/ledger/parameters.go (1)
Parameters(3-7)
internal/api/v1/controllers_accounts_delete_metadata_test.go (2)
internal/log.go (1)
Log(84-96)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/api/v2/controllers_transactions_add_metadata.go (1)
internal/controller/ledger/controller.go (1)
SaveTransactionMetadata(109-112)
internal/api/v1/controllers_transactions_delete_metadata.go (1)
internal/controller/ledger/controller.go (1)
DeleteTransactionMetadata(119-122)
internal/controller/ledger/controller_default_test.go (2)
internal/controller/ledger/controller.go (4)
CreateTransaction(96-100)RevertTransaction(102-107)SaveTransactionMetadata(109-112)DeleteTransactionMetadata(119-122)internal/controller/ledger/parameters.go (1)
Parameters(3-7)
internal/controller/ledger/controller_with_too_many_client_handling_test.go (4)
internal/controller/ledger/errors.go (1)
ErrTooManyClient(16-16)internal/log.go (2)
Log(84-96)CreatedTransaction(199-202)internal/storage/ledger/logs.go (1)
Log(20-26)internal/transaction.go (1)
NewTransaction(228-232)
internal/api/v1/controllers_transactions_delete_metadata_test.go (2)
internal/log.go (1)
Log(84-96)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/api/v2/controllers_accounts_delete_metadata.go (2)
internal/controller/ledger/controller.go (1)
DeleteAccountMetadata(124-127)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/api/v1/controllers_accounts_add_metadata.go (1)
internal/controller/ledger/controller.go (1)
SaveAccountMetadata(114-117)
internal/controller/ledger/controller_with_traces.go (4)
internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (2)
Log(84-96)CreatedTransaction(199-202)internal/storage/ledger/logs.go (1)
Log(20-26)internal/tracing/tracing.go (1)
TraceWithMetric(101-129)
internal/api/v2/controllers_transactions_revert.go (1)
internal/controller/ledger/controller.go (1)
RevertTransaction(102-107)
internal/controller/ledger/controller_default.go (3)
internal/controller/ledger/controller.go (6)
CreateTransaction(96-100)RevertTransaction(102-107)SaveTransactionMetadata(109-112)SaveAccountMetadata(114-117)DeleteTransactionMetadata(119-122)DeleteAccountMetadata(124-127)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (3)
Log(84-96)CreatedTransaction(199-202)RevertedTransaction(329-332)
internal/api/v1/controllers_transactions_revert.go (1)
internal/controller/ledger/controller.go (1)
RevertTransaction(102-107)
internal/api/v1/controllers_transactions_create.go (1)
internal/controller/ledger/controller.go (1)
CreateTransaction(96-100)
internal/controller/system/state_tracker.go (3)
internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (3)
Log(84-96)CreatedTransaction(199-202)RevertedTransaction(329-332)internal/controller/system/controller.go (1)
Controller(41-55)
internal/api/v2/controllers_accounts_add_metadata.go (4)
internal/controller/ledger/controller.go (1)
SaveAccountMetadata(114-117)internal/machine/address.go (1)
Address(7-7)internal/machine/vm/machine.go (1)
Metadata(52-52)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/controller/ledger/log_process.go (2)
internal/log.go (1)
Log(84-96)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/api/v1/controllers_accounts_add_metadata_test.go (1)
internal/log.go (1)
Log(84-96)
internal/api/v2/controllers_transactions_delete_metadata.go (2)
internal/controller/ledger/controller.go (1)
DeleteTransactionMetadata(119-122)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/api/v1/controllers_transactions_add_metadata.go (1)
internal/controller/ledger/controller.go (1)
SaveTransactionMetadata(109-112)
internal/api/v2/controllers_transactions_create.go (1)
internal/controller/ledger/controller.go (1)
CreateTransaction(96-100)
internal/api/common/mocks_ledger_controller_test.go (3)
internal/controller/ledger/controller.go (1)
CreateTransaction(96-100)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/controller/ledger/controller.go (2)
internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (2)
CreatedTransaction(199-202)RevertedTransaction(329-332)
internal/api/v2/controllers_transactions_delete_metadata_test.go (2)
internal/log.go (1)
Log(84-96)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/controller/ledger/controller_with_too_many_client_handling.go (4)
internal/controller/ledger/controller.go (7)
CreateTransaction(96-100)Controller(21-83)RevertTransaction(102-107)SaveTransactionMetadata(109-112)SaveAccountMetadata(114-117)DeleteTransactionMetadata(119-122)DeleteAccountMetadata(124-127)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (3)
Log(84-96)CreatedTransaction(199-202)RevertedTransaction(329-332)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/api/v1/mocks_ledger_controller_test.go (4)
internal/api/common/mocks_ledger_controller_test.go (2)
LedgerControllerMockRecorder(33-35)LedgerController(26-30)internal/controller/ledger/controller.go (7)
Controller(21-83)CreateTransaction(96-100)DeleteAccountMetadata(124-127)DeleteTransactionMetadata(119-122)RevertTransaction(102-107)SaveAccountMetadata(114-117)SaveTransactionMetadata(109-112)internal/volumes.go (2)
BalancesByAssets(97-97)VolumesWithBalanceByAssetByAccount(64-68)internal/controller/ledger/stats.go (1)
Stats(10-13)
internal/api/v2/mocks_ledger_controller_test.go (7)
internal/controller/ledger/controller.go (3)
Controller(21-83)CreateTransaction(96-100)RevertTransaction(102-107)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/account.go (1)
Account(16-26)internal/volumes.go (1)
BalancesByAssets(97-97)internal/controller/ledger/stats.go (1)
Stats(10-13)internal/storage/common/pagination.go (1)
PaginatedQuery(26-29)internal/ledger.go (1)
Ledger(21-30)
internal/api/v1/mocks_system_controller_test.go (5)
internal/api/common/mocks_system_controller_test.go (3)
MockReplicationBackendMockRecorder(30-32)MockReplicationBackend(23-27)SystemController(221-225)internal/ledger.go (2)
Configuration(96-100)Ledger(21-30)internal/controller/system/controller.go (1)
Controller(41-55)internal/storage/common/pagination.go (1)
PaginatedQuery(26-29)internal/storage/system/queries.go (1)
ListLedgersQueryPayload(3-7)
internal/controller/ledger/controller_generated_test.go (2)
internal/controller/ledger/controller.go (7)
Controller(21-83)CreateTransaction(96-100)DeleteAccountMetadata(124-127)DeleteTransactionMetadata(119-122)RevertTransaction(102-107)SaveAccountMetadata(114-117)SaveTransactionMetadata(109-112)internal/controller/ledger/parameters.go (1)
Parameters(3-7)
🪛 markdownlint-cli2 (0.18.1)
pkg/client/docs/models/operations/v2deletetransactionmetadataresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/v2addmetadataontransactionresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/v2deleteaccountmetadataresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/v2addmetadatatoaccountresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/v2createtransactionresponse.md
10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/v2reverttransactionresponse.md
10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
🔇 Additional comments (53)
internal/README.md (1)
727-727: Documentation comment update is clear and appropriate.The updated comment for
IdempotencyHashclearly explains its purpose in validating idempotency key usage. This aligns with the PR objective of adding idempotency support and is an accurate reflection of the field's role in the feature.pkg/client/models/operations/v2deleteaccountmetadata.go (2)
50-50: LGTM! Headers field properly typed.The
Headersfield uses the standard Go type for HTTP headers and aligns with the PR's objective to expose response headers for idempotency tracking.
60-65: LGTM! Proper nil-safety in accessor.The
GetHeaders()method correctly handles nil receivers by returning an empty map, preventing potential nil pointer dereferences for callers.pkg/client/models/operations/v2addmetadataontransaction.go (2)
72-72: LGTM! Headers field correctly added.The
Headersfield uses the standard Go type for HTTP headers (map[string][]string) and appropriately lacks a JSON tag since headers are transport metadata rather than response body content.
82-87: LGTM! GetHeaders() method correctly implemented.The method provides nil-safe access to the Headers field and follows the same pattern as other getters in this file (e.g.,
GetHTTPMeta(),GetRequestBody()). Returning an empty map when the receiver is nil is the appropriate default behavior.pkg/client/models/operations/v2deletetransactionmetadata.go (2)
63-63: Headers field added to support idempotency tracking.The new
Headersfield correctly exposes HTTP response headers, aligning with the PR objective to propagate theIdempotency-Hitheader. Note that unlikeHTTPMeta(line 62), this field lacks a JSON struct tag, meaning it may be included in JSON serialization using the default field name "Headers". This is likely intentional based on the generator configuration, but verify if this matches the expected SDK behavior.
73-78: LGTM! Nil-safe accessor follows established pattern.The
GetHeaders()method correctly implements nil-safety by returning an empty map when the receiver is nil, consistent with the pattern used by other getters in this struct (GetHTTPMeta,GetLedger, etc.).pkg/client/v2.go (1)
2201-2201: LGTM: Idempotency header support correctly implementedThe changes consistently add idempotency header support across mutation endpoints:
- Outbound headers:
utils.PopulateHeaderscorrectly extracts idempotency-related headers from the request object and applies them to the HTTP request- Inbound headers:
res.Headers = httpRes.Headercorrectly propagates response headers (includingIdempotency-Hit) from the HTTP response to the public response object- Success-only propagation: Headers are only propagated on success status codes (200, 201, 204), which is correct
- Complete pairing: Every endpoint that populates outbound headers also propagates inbound headers
The implementation is thread-safe and follows the established patterns in this generated code.
Also applies to: 2315-2316, 2398-2398, 2508-2509, 3227-3227, 3341-3342, 3662-3662, 3776-3777, 3859-3859, 3969-3970, 4058-4059, 4172-4173
internal/log.go (1)
92-92: LGTM: Grammar correctionMinor grammatical improvement to the comment.
internal/api/bulking/mocks.go (1)
1-1: LGTM: Typed mock generation enabledThe
-typedflag addition enables type-safe mock wrappers, consistent with mock generation updates across the codebase.pkg/client/docs/models/operations/v2createtransactionresponse.md (1)
9-10: LGTM: Headers field documentation addedThe documentation correctly reflects the new Headers field. The static analysis warning about a missing link definition for "string" is a false positive—this is type notation, not a markdown link.
pkg/client/models/operations/v2addmetadatatoaccount.go (1)
63-78: LGTM: Headers support added correctlyThe Headers field and GetHeaders method are implemented correctly with proper nil safety.
pkg/client/docs/models/operations/v2reverttransactionresponse.md (1)
9-10: LGTM: Headers field documentation addedConsistent with other response model documentation updates. The static analysis warning is a false positive.
docs/api/README.md (1)
975-980: LGTM: Comprehensive idempotency documentationThe documentation consistently describes the Idempotency-Hit header behavior across all operations that support idempotency (status codes 204, 200, and 201). The Idempotency-Key request header parameters are also properly documented.
Also applies to: 1034-1039, 1491-1496, 1687-1692, 1838-1843
internal/api/v2/controllers_accounts_add_metadata.go (1)
26-39: LGTM: Idempotency hit handling implemented correctlyThe handler now captures the idempotencyHit boolean and sets the Idempotency-Hit header appropriately before writing the response. Ignoring the first return value (the log) is acceptable since this endpoint returns a 204 No Content response.
internal/api/v1/controllers_accounts_delete_metadata_test.go (1)
77-77: LGTM: Test mock updated for new signatureThe mock correctly reflects the expanded DeleteAccountMetadata return signature (Log, bool, error).
pkg/client/models/operations/v2reverttransaction.go (1)
21-25: Idempotency key field and accessor look consistent
IdempotencyKeyis correctly wired as an optional header and the getter mirrors the existing accessor patterns with a nil‑receiver guard. Nothing to change here.Also applies to: 73-79
internal/controller/ledger/controller_with_too_many_client_handling_generated_test.go (1)
5-5: TypedNextmock wrapper is consistent with the interface
MockDelayCalculatorNextCalland itsReturn/Do/DoAndReturnhelpers correctly mirrorNext(int) time.Durationand keep chained expectations (EXPECT().Next(0).Return(...)) working. No issues spotted.Also applies to: 50-77
internal/api/bulking/mocks_ledger_controller_test.go (1)
5-5: Typed LedgerController mocks and idempotency bool plumbing look correctThe generated mocks:
- Match the
ledger0.Controllerinterface for all methods, including the new(…, bool, error)signatures onCreateTransaction,RevertTransaction, and the metadata operations.- Use per‑method wrapper types (
LedgerController<Method>Call) whoseReturn/Do/DoAndReturnsignatures correctly mirror the underlying methods.Given this is generated code and aligns with the interface, I don’t see any issues here. Just ensure the mockgen invocation is tracked so future regenerations stay in sync with the interface.
Also applies to: 59-1028
internal/controller/ledger/controller_generated_test.go (1)
5-5: MockController typed wrappers and bool return extensions are consistentFor
CreateTransaction,RevertTransaction, and the metadata methods, the mocks:
- Return
(log, result, bool, error)/(log, bool, error)with indices and types matching theControllerinterface.- Expose
MockController<Method>Callwrappers whoseReturn/Do/DoAndReturnfunction types line up with the concrete method signatures, including the new bool.Everything here looks coherent with the interface and other mocks; no changes needed.
Also applies to: 59-1027
internal/api/v2/mocks.go (1)
1-2: Typed mock generation directives look good.Using
-typedhere is consistent with the rest of the API mocks and should improve mock type safety.internal/api/v1/controllers_accounts_delete_metadata.go (1)
22-31: Correct adaptation to new DeleteAccountMetadata signature.Unpacking the extra boolean return and ignoring it here is fine for the v1 API, since this endpoint doesn’t expose idempotency headers.
internal/api/v1/controllers_transactions_revert.go (1)
25-32: RevertTransaction call correctly updated for extra return value.Capturing
retwhile discarding log and idempotency flag matches the expanded signature and preserves existing v1 behavior.internal/api/v1/controllers_transactions_delete_metadata.go (1)
27-33: DeleteTransactionMetadata call updated correctly for new triple return.The extra boolean return is safely discarded here, keeping v1 semantics unchanged while aligning with the new controller signature.
internal/api/v1/mocks.go (1)
1-2: v1 mock generation updated consistently with typed mocks.The added
-typedflag for both system and ledger controller mocks is consistent with the rest of the codebase’s mock updates.internal/api/v1/controllers_transactions_delete_metadata_test.go (1)
70-70: Mock signature correctly updated.The mock expectation now returns three values
(*ledger.Log, bool, error)to match the updatedDeleteTransactionMetadatainterface.internal/api/v2/controllers_transactions_delete_metadata.go (1)
26-36: LGTM! Idempotency-Hit header correctly implemented.The handler now captures the
idempotencyHitboolean fromDeleteTransactionMetadataand sets theIdempotency-Hitresponse header appropriately in the success path.internal/api/bulking/bulker.go (6)
143-143: LGTM! Controller call updated correctly.The
CreateTransactioncall now captures the additional boolean return value (idempotency hit flag), which is appropriately ignored in bulk operations.
167-167: LGTM! Controller call updated correctly.The
SaveAccountMetadatacall now captures the additional boolean return value, which is appropriately ignored in bulk operations.
180-180: LGTM! Controller call updated correctly.The
SaveTransactionMetadatacall now captures the additional boolean return value, which is appropriately ignored in bulk operations.
199-199: LGTM! Controller call updated correctly.The
RevertTransactioncall now captures the additional boolean return value, which is appropriately ignored in bulk operations.
228-228: LGTM! Controller call updated correctly.The
DeleteAccountMetadatacall now captures the additional boolean return value, which is appropriately ignored in bulk operations.
242-242: LGTM! Controller call updated correctly.The
DeleteTransactionMetadatacall now captures the additional boolean return value, which is appropriately ignored in bulk operations.internal/api/v1/controllers_accounts_add_metadata.go (1)
38-41: Controller call signature updated correctly.The
SaveAccountMetadatacall now captures the additional boolean return value, which is currently ignored.Confirm whether v1 API handlers should set the
Idempotency-Hitheader like v2 handlers, or if this feature is intentionally v2-only. The verification script from the test file review will help clarify this.internal/api/v1/controllers_transactions_add_metadata_test.go (1)
56-66: SaveTransactionMetadata mock expectation correctly updatedThe gomock
Return(&ledger.Log{}, false, nil)matches the new(*ledger.Log, bool, error)signature and keeps the idempotency flag explicitly false for the nominal path. No further changes needed here.internal/api/v1/controllers_transactions_revert_test.go (1)
75-85: RevertTransaction mock return matches updated signatureThe expectation now returns
(*ledger.Log, *ledger.RevertedTransaction, bool, error)withfalsefor the new idempotency flag. This aligns with the updatedRevertTransactionAPI and keeps the tests’ behavior intact.internal/api/v2/controllers_transactions_revert_test.go (1)
75-84: v2 RevertTransaction mock correctly updated for idempotency flagThe gomock return values now conform to the new
(*ledger.Log, *ledger.RevertedTransaction, bool, error)signature, with the idempotency flag set tofalse. This keeps the existing test logic valid under the new API.internal/api/v2/controllers_transactions_create.go (1)
40-64: Idempotency-Hit handling in createTransaction is wired correctlyThe handler now consumes the new
idempotencyHitboolean fromCreateTransactionand, on success, setsIdempotency-Hit: truebefore writing the response body. This cleanly preserves existing error handling while exposing idempotent replays to clients as intended.internal/api/v2/controllers_accounts_delete_metadata.go (1)
22-37: Idempotency-Hit header propagation from controller looks correctThe handler cleanly threads the new
idempotencyHitflag fromDeleteAccountMetadataand only setsIdempotency-Hit: trueon the successful, idempotent replay path, leaving error handling and first-call behavior unchanged.internal/controller/ledger/log_process_test.go (1)
45-52: Tests updated to new forgeLog signature without behavior changeBoth tests now capture the extra return value from
forgeLogvia_while preserving the existing assertions onerr, which is exactly what's needed for the signature change here.Also applies to: 87-95
internal/controller/ledger/log_process.go (1)
89-133: forgeLog’s idempotencyHit flag is wired correctly across all pathsThe updated
forgeLogcleanly distinguishes between: (a) pre-existing logs found by idempotency key and conflicts resolved viafetchLogWithIK(returnsidempotencyHit=true), and (b) newly forged logs (idempotencyHit=false), while ensuring all error paths returnidempotencyHit=false. This gives callers a reliable signal for header behavior without changing the underlying deadlock/idempotency handling.internal/controller/ledger/controller_default_test.go (1)
71-77: Controller default tests correctly adapted to new idempotency-aware signaturesThe tests now destructure the extended controller returns (extra
boolbeforeerror) while preserving their original success assertions, which keeps them aligned with the new API surface without altering behavior.Also applies to: 127-133, 172-178, 215-221
internal/api/v2/controllers_bulk_test.go (1)
76-88: Bulk controller mocks correctly updated for idempotency booleanThe bulk tests’ expectations now match the extended controller signatures, returning
falseforidempotencyHiton fresh operations and threading the flag through all success and error cases without changing existing assertions on bulk results or error handling.Also applies to: 120-132, 150-162, 179-203, 227-238, 277-300, 351-386, 425-455, 515-540
internal/controller/system/state_tracker.go (1)
114-197: State tracker correctly threads idempotencyHit through handleStateThe facade methods (
CreateTransaction,RevertTransaction,Save*Metadata,Delete*Metadata) now consistently captureidempotencyHitfrom the underlyingledgercontroller.ControllerinsidehandleStateand return it alongside the existing outputs.The control flow (lock, optional TX, commit/rollback) is unchanged apart from the extra boolean, so external behavior should match the previous implementation plus the new flag. No concurrency or transaction‑lifecycle issues introduced here.
internal/controller/ledger/controller_with_traces.go (2)
413-456: Save*Metadata traced wrappers preserve behavior while surfacing idempotencyHitBoth
SaveTransactionMetadataandSaveAccountMetadatawrap the underlying calls inTraceWithMetric[*ledger.Log], assignlogandidempotencyHitinside the closure, and on exit return(nil, false, err)or(log, idempotencyHit, nil).This preserves previous tracing/latency behavior and simply adds the extra boolean without changing DryRun or error semantics.
458-501: Delete*Metadata traced wrappers match the new signature and error semantics
DeleteTransactionMetadataandDeleteAccountMetadatafollow the same pattern as the save variants: locallog,idempotencyHit, wrapped inTraceWithMetric[*ledger.Log], and they zero the outputs on error.Propagation of
idempotencyHitfromunderlyingthrough the tracing layer looks consistent and correct.internal/controller/ledger/controller_default.go (1)
415-565: DefaultController wrappers cleanly surface idempotencyHit from logProcessorThe public methods now delegate as follows:
CreateTransaction/RevertTransactionreturn the full tuple fromcreateTransactionLp/revertTransactionLp.forgeLog, which now includes thebool.Save*MetadataandDelete*MetadataunwrapforgeLogas(log, _, idempotencyHit, err)and expose(log, idempotencyHit, err).Assuming
forgeLog’s new signature is(*ledger.Log, *Payload, bool, error)(as implied by usage), this is a straightforward and consistent way to surface idempotencyHit without touching the core business logic in the helpers.No functional issues spotted here.
internal/api/common/mocks_ledger_controller_test.go (1)
109-422: Mocks updated to match new controller signatures (including idempotencyHit)The
LedgerControllermock methods now mirror the updated controller interface:
CreateTransaction/RevertTransactionreturn four values and correctly unpackret[2]asboolandret[3]aserror.SaveAccountMetadata,SaveTransactionMetadata,DeleteAccountMetadata, andDeleteTransactionMetadataall return(*ledger.Log, bool, error)and unpack the new boolean at index 1.This keeps tests aligned with the new API surface and ensures the idempotencyHit flag can be exercised in expectations and returns. No issues from the gomock usage itself.
internal/api/v2/mocks_system_controller_test.go (1)
55-506: Typed gomock wrappers for v2 SystemController/ReplicationBackend look consistentRecorder methods and the
*...Callwrapper types correctly mirror the underlying method signatures (params and return types) and provide typedReturn/Do/DoAndReturnhelpers. This should improve test ergonomics without changing behavior.Also applies to: 532-1299
internal/controller/ledger/controller_with_too_many_client_handling.go (1)
46-58: Idempotency hit flag is correctly threaded through retry wrapperFor CreateTransaction, RevertTransaction, and the Save*/Delete* metadata methods, the new
idempotencyHit boolis:
- Declared alongside the existing results,
- Assigned inside the
handleRetryclosure from the underlyingControllercall, and- Returned from the wrapper in the same positional order as defined on the
Controllerinterface.This preserves the retry behavior while exposing idempotency information cleanly.
Also applies to: 60-73, 75-88, 89-102, 103-116, 117-129
internal/api/v1/mocks_system_controller_test.go (1)
55-506: v1 SystemController/ReplicationBackend typed mocks match interface surfaceThe recorder methods and
*...Callwrapper types correctly mirror the v1 system controller and replication backend interfaces and expose typedReturn/Do/DoAndReturnhelpers. The generated code looks consistent and safe to rely on in tests.Also applies to: 532-1299
internal/api/v1/mocks_ledger_controller_test.go (1)
205-245: v1 LedgerController mocks correctly incorporate idempotency booleanFor
CreateTransaction,RevertTransaction, and the Save*/Delete* metadata methods, the mock methods and theirLedgerController*Callwrappers:
- Use the updated return shapes (
(*ledger.Log, *ledger.CreatedTransaction, bool, error)and(*ledger.Log, bool, error)),- Read the bool from the correct index in
ret[...], and- Expose matching typed
Return/Do/DoAndReturnsignatures.This keeps the mocks in sync with the updated controller interface and should work smoothly with idempotency-aware tests.
Also applies to: 246-325, 871-910, 950-1028
internal/api/v2/mocks_ledger_controller_test.go (1)
205-245: v2 LedgerController mocks are correctly updated for idempotency-aware signaturesThe v2 mocks mirror the controller interface changes:
CreateTransaction/RevertTransactionreturn(*ledger.Log, *ledger.CreatedTransaction|*ledger.RevertedTransaction, bool, error)and read the bool and error from the correct positions.- Save/Delete metadata methods return
(*ledger.Log, bool, error)with matching indices.- The associated
LedgerController*Callwrappers expose typedReturn/Do/DoAndReturnwith the same signatures.This keeps the v2 mock surface consistent with the new idempotency semantics.
Also applies to: 246-325, 871-910, 950-1028
pkg/client/docs/models/operations/v2deletetransactionmetadataresponse.md
Show resolved
Hide resolved
|
@cubic-dev-ai review this pull request |
@flemzord I've started the AI code review. It'll take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 71 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/api/v1/controllers_transactions_delete_metadata.go">
<violation number="1" location="internal/api/v1/controllers_transactions_delete_metadata.go:27">
The idempotency-hit flag returned by DeleteTransactionMetadata is ignored here, so the v1 endpoint never emits the new Idempotency-Hit header even when a request replays a previous command.</violation>
</file>
<file name="internal/api/v1/controllers_accounts_add_metadata.go">
<violation number="1" location="internal/api/v1/controllers_accounts_add_metadata.go:38">
The idempotency hit flag returned by `SaveAccountMetadata` is discarded, so this v1 endpoint never sets the required Idempotency-Hit response header.</violation>
</file>
<file name="internal/api/v1/controllers_transactions_revert.go">
<violation number="1" location="internal/api/v1/controllers_transactions_revert.go:25">
The controller drops the new `idempotencyHit` return value, so the v1 revert endpoint never emits the required `Idempotency-Hit` response header.</violation>
</file>
<file name="internal/api/v1/controllers_transactions_add_metadata.go">
<violation number="1" location="internal/api/v1/controllers_transactions_add_metadata.go:33">
Discarding the idempotencyHit return value means this controller never sets the Idempotency-Hit response header, so clients cannot tell when their request was served from an idempotency cache.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
gfyrag
force-pushed
the
feat/idempotency-hit
branch
from
18d8eee to
6bcf605
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/client/models/operations/createtransaction.go (1)
63-68: Consider normalizing nil Headers to an empty map for stronger nil-safety
GetHeadersreturns an empty map only when the receiver isnil; ifois non‑nil butHeadersisnil, callers still getniland must handle that case separately. If you want fully nil‑safe semantics, you could also normalizenilo.Headersto an empty map:func (o *CreateTransactionResponse) GetHeaders() map[string][]string { - if o == nil { - return map[string][]string{} - } - return o.Headers + if o == nil { + return map[string][]string{} + } + if o.Headers == nil { + return map[string][]string{} + } + return o.Headers }This is optional and mainly about ergonomic consistency for callers.
pkg/client/models/operations/addmetadataontransaction.go (1)
64-69: Nil‑receiver behavior inGetHeadersis fine; consider consistency on nil vs empty maps
GetHeaderssafely handles a nil receiver and returns an empty map, which is convenient for callers that iterate without nil checks. Note that ifo != nilbuto.Headers == nil, this method will returnnilrather than an empty map, so callers that mutate the returned map might still need to defensively initialize it. If you want fully uniform behavior, you could normalizeniltomap[string][]string{}in the non‑nil branch as well, but this is optional.internal/api/v1/controllers_accounts_delete_metadata.go (1)
22-37: Idempotency-Hit header correctly added for deleteAccountMetadata
DeleteAccountMetadata’s newidempotencyHitflag is captured, errors are handled first, andIdempotency-Hitis only set on successful idempotent hits. This is consistent with the other controllers in the PR.If you touch this again, you could optionally pull
ctx := r.Context()andl := common.LedgerFromContext(ctx)for slightly clearer flow, but it’s not required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
openapi.yamlis excluded by!**/*.yamlopenapi/v1.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.logpkg/client/speakeasyusagegen/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.log
📒 Files selected for processing (15)
internal/api/v1/controllers_accounts_add_metadata.go(1 hunks)internal/api/v1/controllers_accounts_delete_metadata.go(1 hunks)internal/api/v1/controllers_transactions_add_metadata.go(2 hunks)internal/api/v1/controllers_transactions_create.go(4 hunks)internal/api/v1/controllers_transactions_delete_metadata.go(1 hunks)internal/api/v1/controllers_transactions_revert.go(2 hunks)pkg/client/docs/models/operations/addmetadataontransactionresponse.md(1 hunks)pkg/client/docs/models/operations/addmetadatatoaccountresponse.md(1 hunks)pkg/client/docs/models/operations/createtransactionresponse.md(1 hunks)pkg/client/docs/models/operations/reverttransactionresponse.md(1 hunks)pkg/client/models/operations/addmetadataontransaction.go(2 hunks)pkg/client/models/operations/addmetadatatoaccount.go(2 hunks)pkg/client/models/operations/createtransaction.go(2 hunks)pkg/client/models/operations/reverttransaction.go(2 hunks)pkg/client/v1.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/client/v1.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/api/v1/controllers_transactions_add_metadata.go
- internal/api/v1/controllers_transactions_create.go
- internal/api/v1/controllers_accounts_add_metadata.go
🧰 Additional context used
🧬 Code graph analysis (3)
internal/api/v1/controllers_accounts_delete_metadata.go (2)
internal/controller/ledger/controller.go (1)
DeleteAccountMetadata(124-127)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/api/v1/controllers_transactions_revert.go (1)
internal/controller/ledger/controller.go (1)
RevertTransaction(102-107)
internal/api/v1/controllers_transactions_delete_metadata.go (2)
internal/controller/ledger/controller.go (1)
DeleteTransactionMetadata(119-122)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
🪛 markdownlint-cli2 (0.18.1)
pkg/client/docs/models/operations/addmetadataontransactionresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/createtransactionresponse.md
10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (7)
pkg/client/models/operations/createtransaction.go (1)
42-47: Headers field addition looks consistent with response modelingAdding
Headers map[string][]stringonCreateTransactionResponsealigns with the pattern of exposing HTTP headers on client responses and keeps types straightforward for consumers. No issues from a correctness or API-design standpoint here.pkg/client/models/operations/addmetadataontransaction.go (1)
52-55: AddingHeadersfield on the response is reasonable and consistentExposing
Headers map[string][]stringonAddMetadataOnTransactionResponsefits the use case of surfacing HTTP headers (e.g., idempotency information) to SDK consumers and aligns with the existingHTTPMetapattern. No issues from a type/visibility perspective.pkg/client/models/operations/addmetadatatoaccount.go (2)
45-45: LGTM! Headers field properly added.The new
Headersfield correctly captures HTTP response headers to support the idempotency-hit feature. The lack of struct tags is consistent with the existingHTTPMetafield pattern.
55-60: LGTM! Getter method follows best practices.The
GetHeaders()method is correctly implemented with nil-safe behavior and follows the same pattern as other getters in this file. Returning an empty map instead of nil is good defensive programming.internal/api/v1/controllers_transactions_delete_metadata.go (1)
27-37: Idempotency-Hit propagation for deleteTransactionMetadata looks correct
DeleteTransactionMetadata’s newidempotencyHitreturn is captured, errors are handled first, andIdempotency-Hit: trueis only set on successful idempotent hits. This matches the intended pattern used elsewhere in the PR.internal/api/v1/controllers_transactions_revert.go (1)
25-48: RevertTransaction idempotency wiring is consistent and safeThe handler now unpacks
idempotencyHitfromRevertTransactionand setsIdempotency-Hit: trueonly after a successful call. Error mapping and the 201 response body remain unchanged, so behavior is backward compatible apart from the new header.pkg/client/models/operations/reverttransaction.go (1)
56-57: Generated Headers field and accessor look consistent with the new API surfaceAdding
Headers map[string][]stringtoRevertTransactionResponseand exposing it viaGetHeaders()(with a nil-safe receiver check) matches the intended API change and aligns with how other response metadata is modeled. Given this file is generated, leaving the implementation as-is is appropriate.Also applies to: 73-78
gfyrag
force-pushed
the
feat/idempotency-hit
branch
from
6bcf605 to
5229294
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
pkg/client/docs/models/operations/reverttransactionresponse.md (1)
10-10: Fix Headers type formatting and align with Go model (duplicate from prior review).This is a duplicate of the earlier review comment. The
Headerstype documentation at line 10 showsmap[string][]*string*, which:
- Triggers a markdownlint MD052 error (undefined reference
[string])- Mismatches the actual Go client model:
Headers map[string][]stringApply the suggested fix from the prior review:
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | +| `Headers` | `map[string][]string` | :heavy_check_mark: | N/A |This syncs the docs with the code and resolves the markdown formatting issue.
pkg/client/docs/models/operations/addmetadatatoaccountresponse.md (1)
9-9: Improve the Headers field documentation — duplicate of past review comment.As flagged in the prior review, the
Headersfield description remains "N/A", which doesn't explain its purpose to API consumers. Given the PR objective to expose the Idempotency-Hit header, clarify the field's content.If this file is auto-generated, update the generator or template to emit a meaningful description for the Headers field. If manually maintained, update the description to:
Response HTTP headers, including Idempotency-Hit when the request matched a previous idempotent operation.pkg/client/docs/models/operations/v2deleteaccountmetadataresponse.md (1)
9-9: Fix markdown formatting for the type syntax.The asterisks in
*string*are being interpreted as markdown italic markers, causing a linting error. The type should be formatted as a proper code token without unescaped special characters.Apply this diff to fix the formatting (assuming the intended type is
map[string][]stringfor standard HTTP headers):-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | +| `Headers` | map[string][]string | :heavy_check_mark: | N/A |If the type is intended to be pointers to strings, escape the asterisks:
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | +| `Headers` | map[string][]\*string | :heavy_check_mark: | N/A |pkg/client/docs/models/operations/v2deletetransactionmetadataresponse.md (1)
9-9: Fix markdown formatting for Headers type.The
Headerstype annotation has formatting issues:map[string][]*string*contains a stray trailing asterisk and triggers markdown linting errors. Wrap the entire type in backticks:`map[string][]*string`.pkg/client/docs/models/operations/createtransactionresponse.md (1)
10-10: Fix markdown formatting for Headers type.The
Headerstype annotation has a stray trailing asterisk (map[string][]*string*) that causes markdown linting errors. Wrap the type in backticks and remove the trailing asterisk:`map[string][]*string`.pkg/client/docs/models/operations/addmetadataontransactionresponse.md (1)
8-9: FixHeaderstype formatting - markdown interprets[string]as a reference link.The type
map[string][]*string*has formatting issues:[string]triggers markdownlint MD052 and the asterisks render as italics. The Go client usesmap[string][]string.This was flagged in a previous review. Apply the fix:
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | +| `Headers` | `map[string][]string` | :heavy_check_mark: | N/A |pkg/client/docs/models/operations/v2addmetadatatoaccountresponse.md (1)
8-9: FixHeaderstype formatting.Same markdown formatting issue as other response docs -
map[string][]*string*should be`map[string][]string`to match the Go client type and avoid markdownlint MD052.This was flagged in a previous review.
🧹 Nitpick comments (7)
internal/api/v1/controllers_transactions_delete_metadata_test.go (1)
70-70: Consider adding test coverage for idempotency hit behavior.While the mock is correctly updated to match the new signature, there are no test cases that verify the behavior when idempotency-hit is true. This leaves the header-setting logic untested in this endpoint.
Consider adding a test case that verifies the Idempotency-Hit header is set correctly:
+ { + name: "with idempotency hit", + expectBackendCall: true, + returnIdempotencyHit: true, + },Then in the test body:
if tc.expectBackendCall { ledgerController.EXPECT(). DeleteTransactionMetadata(gomock.Any(), ledgercontroller.Parameters[ledgercontroller.DeleteTransactionMetadata]{ Input: ledgercontroller.DeleteTransactionMetadata{ TransactionID: 1, Key: "foo", }, }). - Return(&ledger.Log{}, false, tc.returnErr) + Return(&ledger.Log{}, tc.returnIdempotencyHit, tc.returnErr) }And verify the header:
if tc.expectedStatusCode == 0 || tc.expectedStatusCode == http.StatusNoContent { require.Equal(t, http.StatusNoContent, rec.Code) + if tc.returnIdempotencyHit { + require.Equal(t, "true", rec.Header().Get("Idempotency-Hit")) + }internal/api/v2/controllers_transactions_create_test.go (1)
415-417: Consider adding test coverage for idempotency hit behavior.Similar to other endpoints, there are no test cases that verify the Idempotency-Hit header is set correctly when idempotency-hit is true. Consider adding a test case that mocks a successful idempotency hit and verifies the response header.
internal/api/v1/controllers_transactions_revert_test.go (1)
84-84: Consider adding test coverage for idempotency hit behavior.The mock is correctly updated to match the new signature, but there are no test cases that verify the Idempotency-Hit header behavior for this endpoint.
internal/api/v2/controllers_transactions_revert_test.go (1)
83-83: Consider adding test coverage for idempotency hit behavior.The mock is correctly updated to match the new signature, but there are no test cases that verify the Idempotency-Hit header is set correctly when idempotency-hit is true.
internal/api/v1/controllers_accounts_add_metadata_test.go (1)
100-100: Consider adding test coverage for idempotency hit scenario.The test mocks now return
falseforidempotencyHit, but there's no test case that verifies theIdempotency-Hitheader is set whenidempotencyHitistrue. Consider adding a test case that:
- Mocks
SaveAccountMetadatato returntrueforidempotencyHit- Asserts that the response includes the
Idempotency-Hit: trueheaderinternal/api/v2/controllers_accounts_add_metadata_test.go (1)
72-72: Consider adding test coverage for idempotency hit scenario.Similar to the v1 test, consider adding a test case that verifies the
Idempotency-Hitheader is set whenidempotencyHitistrue.internal/controller/ledger/controller.go (1)
44-52: Consider documenting the newidempotencyHitreturn value.The existing comments document possible errors but don't explain the new boolean return. Consider adding a brief note, e.g.:
// CreateTransaction accept a numscript script and returns a transaction // It can return following errors: // * ErrCompilationFailed // * ErrMetadataOverride // * ErrInvalidVars // * ErrTransactionReferenceConflict // * ErrIdempotencyKeyConflict // * ErrInsufficientFunds +// The bool return indicates whether this was an idempotent cache hit (true) or a fresh operation (false). CreateTransaction(ctx context.Context, parameters Parameters[CreateTransaction]) (*ledger.Log, *ledger.CreatedTransaction, bool, error)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
openapi.yamlis excluded by!**/*.yamlopenapi/v1.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.logpkg/client/speakeasyusagegen/.speakeasy/logs/naming.logis excluded by!**/*.log,!**/*.log
📒 Files selected for processing (75)
docs/api/README.md(7 hunks)internal/README.md(1 hunks)internal/api/bulking/bulker.go(6 hunks)internal/api/bulking/bulker_test.go(13 hunks)internal/api/bulking/mocks.go(1 hunks)internal/api/bulking/mocks_ledger_controller_test.go(20 hunks)internal/api/common/mocks_ledger_controller_test.go(6 hunks)internal/api/v1/controllers_accounts_add_metadata.go(1 hunks)internal/api/v1/controllers_accounts_add_metadata_test.go(1 hunks)internal/api/v1/controllers_accounts_delete_metadata.go(1 hunks)internal/api/v1/controllers_accounts_delete_metadata_test.go(1 hunks)internal/api/v1/controllers_transactions_add_metadata.go(2 hunks)internal/api/v1/controllers_transactions_add_metadata_test.go(1 hunks)internal/api/v1/controllers_transactions_create.go(4 hunks)internal/api/v1/controllers_transactions_create_test.go(1 hunks)internal/api/v1/controllers_transactions_delete_metadata.go(1 hunks)internal/api/v1/controllers_transactions_delete_metadata_test.go(1 hunks)internal/api/v1/controllers_transactions_revert.go(2 hunks)internal/api/v1/controllers_transactions_revert_test.go(1 hunks)internal/api/v1/mocks.go(1 hunks)internal/api/v1/mocks_ledger_controller_test.go(20 hunks)internal/api/v1/mocks_system_controller_test.go(33 hunks)internal/api/v2/controllers_accounts_add_metadata.go(1 hunks)internal/api/v2/controllers_accounts_add_metadata_test.go(1 hunks)internal/api/v2/controllers_accounts_delete_metadata.go(1 hunks)internal/api/v2/controllers_accounts_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_bulk_test.go(13 hunks)internal/api/v2/controllers_transactions_add_metadata.go(2 hunks)internal/api/v2/controllers_transactions_add_metadata_test.go(1 hunks)internal/api/v2/controllers_transactions_create.go(2 hunks)internal/api/v2/controllers_transactions_create_test.go(1 hunks)internal/api/v2/controllers_transactions_delete_metadata.go(1 hunks)internal/api/v2/controllers_transactions_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_transactions_revert.go(2 hunks)internal/api/v2/controllers_transactions_revert_test.go(1 hunks)internal/api/v2/mocks.go(1 hunks)internal/api/v2/mocks_ledger_controller_test.go(20 hunks)internal/api/v2/mocks_system_controller_test.go(33 hunks)internal/controller/ledger/controller.go(2 hunks)internal/controller/ledger/controller_default.go(6 hunks)internal/controller/ledger/controller_default_test.go(4 hunks)internal/controller/ledger/controller_generated_test.go(20 hunks)internal/controller/ledger/controller_with_events.go(6 hunks)internal/controller/ledger/controller_with_too_many_client_handling.go(2 hunks)internal/controller/ledger/controller_with_too_many_client_handling_generated_test.go(2 hunks)internal/controller/ledger/controller_with_too_many_client_handling_test.go(4 hunks)internal/controller/ledger/controller_with_traces.go(1 hunks)internal/controller/ledger/log_process.go(2 hunks)internal/controller/ledger/log_process_test.go(2 hunks)internal/controller/system/state_tracker.go(1 hunks)internal/log.go(1 hunks)pkg/client/docs/models/operations/addmetadataontransactionresponse.md(1 hunks)pkg/client/docs/models/operations/addmetadatatoaccountresponse.md(1 hunks)pkg/client/docs/models/operations/createtransactionresponse.md(1 hunks)pkg/client/docs/models/operations/reverttransactionresponse.md(1 hunks)pkg/client/docs/models/operations/v2addmetadataontransactionresponse.md(1 hunks)pkg/client/docs/models/operations/v2addmetadatatoaccountresponse.md(1 hunks)pkg/client/docs/models/operations/v2createtransactionresponse.md(1 hunks)pkg/client/docs/models/operations/v2deleteaccountmetadataresponse.md(1 hunks)pkg/client/docs/models/operations/v2deletetransactionmetadataresponse.md(1 hunks)pkg/client/docs/models/operations/v2reverttransactionrequest.md(1 hunks)pkg/client/docs/models/operations/v2reverttransactionresponse.md(1 hunks)pkg/client/models/operations/addmetadataontransaction.go(2 hunks)pkg/client/models/operations/addmetadatatoaccount.go(2 hunks)pkg/client/models/operations/createtransaction.go(2 hunks)pkg/client/models/operations/reverttransaction.go(2 hunks)pkg/client/models/operations/v2addmetadataontransaction.go(2 hunks)pkg/client/models/operations/v2addmetadatatoaccount.go(2 hunks)pkg/client/models/operations/v2createtransaction.go(2 hunks)pkg/client/models/operations/v2deleteaccountmetadata.go(2 hunks)pkg/client/models/operations/v2deletetransactionmetadata.go(2 hunks)pkg/client/models/operations/v2reverttransaction.go(4 hunks)pkg/client/v1.go(4 hunks)pkg/client/v2.go(7 hunks)test/e2e/api_idempotency_hit_header_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/log.go
🚧 Files skipped from review as they are similar to previous changes (22)
- pkg/client/models/operations/v2createtransaction.go
- internal/api/v2/controllers_transactions_add_metadata_test.go
- pkg/client/models/operations/v2deleteaccountmetadata.go
- pkg/client/models/operations/v2reverttransaction.go
- internal/README.md
- pkg/client/models/operations/addmetadatatoaccount.go
- internal/api/v1/controllers_transactions_create_test.go
- internal/api/bulking/bulker.go
- internal/api/v2/controllers_transactions_create.go
- internal/api/v2/controllers_accounts_delete_metadata_test.go
- pkg/client/v2.go
- internal/api/bulking/bulker_test.go
- pkg/client/models/operations/v2addmetadataontransaction.go
- pkg/client/docs/models/operations/v2addmetadataontransactionresponse.md
- internal/api/v2/controllers_transactions_delete_metadata_test.go
- internal/controller/ledger/controller_default_test.go
- pkg/client/models/operations/reverttransaction.go
- internal/controller/ledger/log_process_test.go
- pkg/client/docs/models/operations/v2reverttransactionrequest.md
- internal/api/v1/mocks.go
- internal/api/v1/controllers_accounts_delete_metadata_test.go
- pkg/client/v1.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-29T11:24:28.923Z
Learnt from: gfyrag
Repo: formancehq/ledger PR: 892
File: internal/controller/ledger/controller_default.go:196-196
Timestamp: 2025-04-29T11:24:28.923Z
Learning: In the ledger Import function, it's critical to maintain proper log ID tracking by updating lastLogID with the current log.ID after each processed log, rather than setting it to nil. This ensures the system can properly validate the ordering of logs and prevent duplicate or out-of-order processing, which is essential for maintaining data integrity in the ledger.
Applied to files:
internal/controller/ledger/log_process.go
🧬 Code graph analysis (24)
internal/api/v1/controllers_transactions_add_metadata_test.go (1)
internal/log.go (1)
Log(84-96)
internal/api/v2/controllers_accounts_delete_metadata.go (3)
internal/controller/ledger/controller.go (1)
DeleteAccountMetadata(124-127)internal/machine/address.go (1)
Address(7-7)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/api/v2/controllers_accounts_add_metadata_test.go (1)
internal/log.go (1)
Log(84-96)
internal/api/v1/controllers_transactions_add_metadata.go (2)
internal/controller/ledger/controller.go (1)
SaveTransactionMetadata(109-112)internal/machine/vm/machine.go (1)
Metadata(52-52)
internal/api/v1/controllers_accounts_add_metadata_test.go (1)
internal/log.go (1)
Log(84-96)
internal/api/v1/controllers_accounts_delete_metadata.go (2)
internal/controller/ledger/controller.go (1)
DeleteAccountMetadata(124-127)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/api/v2/controllers_transactions_add_metadata.go (2)
internal/controller/ledger/controller.go (1)
SaveTransactionMetadata(109-112)internal/machine/vm/machine.go (1)
Metadata(52-52)
internal/api/v1/controllers_transactions_create.go (1)
internal/controller/ledger/controller.go (1)
CreateTransaction(96-100)
internal/api/v1/controllers_transactions_delete_metadata.go (2)
internal/controller/ledger/controller.go (1)
DeleteTransactionMetadata(119-122)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/api/v2/controllers_transactions_delete_metadata.go (2)
internal/controller/ledger/controller.go (1)
DeleteTransactionMetadata(119-122)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/controller/ledger/controller.go (3)
internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (3)
Log(84-96)CreatedTransaction(199-202)RevertedTransaction(329-332)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/controller/ledger/controller_with_traces.go (5)
internal/controller/ledger/controller.go (6)
CreateTransaction(96-100)RevertTransaction(102-107)SaveTransactionMetadata(109-112)SaveAccountMetadata(114-117)DeleteTransactionMetadata(119-122)DeleteAccountMetadata(124-127)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (3)
Log(84-96)CreatedTransaction(199-202)RevertedTransaction(329-332)internal/storage/ledger/logs.go (1)
Log(20-26)internal/tracing/tracing.go (1)
TraceWithMetric(101-129)
internal/api/v2/mocks_system_controller_test.go (4)
internal/api/common/mocks_system_controller_test.go (4)
MockReplicationBackendMockRecorder(30-32)MockReplicationBackend(23-27)SystemControllerMockRecorder(228-230)SystemController(221-225)internal/ledger.go (2)
Configuration(96-100)Ledger(21-30)internal/controller/ledger/controller.go (1)
Controller(21-83)internal/controller/system/controller.go (1)
Controller(41-55)
internal/api/v1/controllers_accounts_add_metadata.go (4)
internal/controller/ledger/controller.go (1)
SaveAccountMetadata(114-117)internal/machine/address.go (1)
Address(7-7)internal/machine/vm/machine.go (1)
Metadata(52-52)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/api/common/mocks_ledger_controller_test.go (3)
internal/controller/ledger/controller.go (1)
CreateTransaction(96-100)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/api/v2/controllers_transactions_revert.go (1)
internal/controller/ledger/controller.go (1)
RevertTransaction(102-107)
internal/controller/ledger/controller_with_too_many_client_handling.go (3)
internal/controller/ledger/controller.go (7)
CreateTransaction(96-100)Controller(21-83)RevertTransaction(102-107)SaveTransactionMetadata(109-112)SaveAccountMetadata(114-117)DeleteTransactionMetadata(119-122)DeleteAccountMetadata(124-127)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (3)
Log(84-96)CreatedTransaction(199-202)RevertedTransaction(329-332)
internal/api/v2/controllers_accounts_add_metadata.go (2)
internal/controller/ledger/controller.go (1)
SaveAccountMetadata(114-117)internal/api/common/errors.go (1)
HandleCommonWriteErrors(45-56)
internal/api/v1/controllers_transactions_revert.go (1)
internal/controller/ledger/controller.go (1)
RevertTransaction(102-107)
internal/api/v1/mocks_system_controller_test.go (5)
internal/api/common/mocks_system_controller_test.go (3)
MockReplicationBackendMockRecorder(30-32)MockReplicationBackend(23-27)SystemController(221-225)internal/ledger.go (2)
Configuration(96-100)Ledger(21-30)internal/controller/system/controller.go (1)
Controller(41-55)internal/storage/common/pagination.go (1)
PaginatedQuery(26-29)internal/storage/system/queries.go (1)
ListLedgersQueryPayload(3-7)
internal/api/bulking/mocks_ledger_controller_test.go (6)
internal/api/common/mocks_ledger_controller_test.go (2)
LedgerControllerMockRecorder(33-35)LedgerController(26-30)internal/api/v1/mocks_ledger_controller_test.go (2)
LedgerControllerMockRecorder(33-35)LedgerController(26-30)internal/controller/ledger/controller.go (7)
Controller(21-83)CreateTransaction(96-100)DeleteAccountMetadata(124-127)DeleteTransactionMetadata(119-122)RevertTransaction(102-107)SaveAccountMetadata(114-117)SaveTransactionMetadata(109-112)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/storage/common/pagination.go (1)
PaginatedQuery(26-29)internal/ledger.go (1)
Ledger(21-30)
internal/api/v2/mocks_ledger_controller_test.go (2)
internal/api/common/mocks_ledger_controller_test.go (2)
LedgerControllerMockRecorder(33-35)LedgerController(26-30)internal/controller/ledger/controller.go (7)
Controller(21-83)CreateTransaction(96-100)DeleteAccountMetadata(124-127)DeleteTransactionMetadata(119-122)RevertTransaction(102-107)SaveAccountMetadata(114-117)SaveTransactionMetadata(109-112)
internal/controller/ledger/log_process.go (2)
internal/log.go (1)
Log(84-96)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/controller/ledger/controller_generated_test.go (2)
internal/controller/ledger/controller.go (7)
Controller(21-83)CreateTransaction(96-100)DeleteAccountMetadata(124-127)DeleteTransactionMetadata(119-122)RevertTransaction(102-107)SaveAccountMetadata(114-117)SaveTransactionMetadata(109-112)internal/controller/ledger/parameters.go (1)
Parameters(3-7)
🪛 markdownlint-cli2 (0.18.1)
pkg/client/docs/models/operations/v2createtransactionresponse.md
10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/createtransactionresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/addmetadataontransactionresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/reverttransactionresponse.md
10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/v2deleteaccountmetadataresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/v2deletetransactionmetadataresponse.md
9-9: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
pkg/client/docs/models/operations/v2reverttransactionresponse.md
10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (68)
internal/api/bulking/mocks.go (1)
1-1: LGTM: Typed mock generation enabled.The addition of the
-typedflag improves type safety by generating per-call wrapper types for the mocks, which aligns with the broader mock updates across the PR.internal/api/v1/controllers_accounts_delete_metadata.go (2)
22-29: LGTM: Idempotency indicator correctly unpacked.The handler now correctly receives the
idempotencyHitboolean fromDeleteAccountMetadata, maintaining proper error handling flow.
35-37: LGTM: Idempotency header correctly set.The
Idempotency-Hitheader is properly set before the response is written, following the correct HTTP header emission pattern.pkg/client/models/operations/createtransaction.go (2)
46-46: LGTM: Headers field appropriately added.The
Headersfield follows standard Go conventions for exposing HTTP response headers in client models.
63-68: LGTM: Nil-safe accessor follows conventions.The
GetHeadersmethod provides appropriate nil-safe access to the headers, consistent with other accessor patterns in this generated code.pkg/client/models/operations/addmetadataontransaction.go (2)
54-54: LGTM: Headers field consistently added.The
Headersfield follows the same pattern used across other response models in the client, providing standard HTTP header access.
64-69: LGTM: Nil-safe accessor matches established pattern.The
GetHeadersmethod provides consistent nil-safe access, matching the pattern used throughout the generated client code.pkg/client/models/operations/v2deletetransactionmetadata.go (1)
61-78: LGTM - Generated code with proper nil-safety.The added
Headersfield andGetHeaders()accessor follow the established pattern for exposing HTTP response headers. The nil-safe implementation returning an empty map is appropriate for a generated client model.test/e2e/api_idempotency_hit_header_test.go (7)
1-47: Well-structured e2e test setup for idempotency header verification.Good use of Ginkgo's deferred setup patterns with
UseTemplatedDatabase()andDeferTestServer(). TheBeforeEachcorrectly creates the ledger before each spec.
49-98: LGTM - CreateTransaction idempotency tests.The test correctly verifies:
- First call returns no
Idempotency-Hitheader- Second call with same key returns
Idempotency-Hit: true
100-171: LGTM - AddMetadataOnTransaction idempotency tests.Good setup creating a transaction in
BeforeEachbefore testing metadata operations. Uses distinct idempotency keys (metadata-idempotency-key-header-1vs-2) for the two test cases to ensure isolation.
173-258: LGTM - DeleteTransactionMetadata idempotency tests.Proper setup chain: creates transaction, adds metadata, then tests delete operations with idempotency keys.
260-348: LGTM - RevertTransaction idempotency tests.Correctly creates separate transactions within each
Itblock since a transaction can only be reverted once. This isolation is necessary for the revert operation semantics.
350-406: LGTM - AddMetadataToAccount idempotency tests.Uses distinct account addresses (
account-header-test-1vs-2) and idempotency keys per test case for proper isolation.
408-478: LGTM - DeleteAccountMetadata idempotency tests.Proper setup adds metadata in
BeforeEach, then tests delete operations. The shared account addressaccount-delete-header-testworks here since theBeforeEachruns before eachItblock.internal/api/v2/mocks.go (1)
1-3: Slightly imprecise description but correct implementation.The review comment states methods return
(result, idempotencyHit bool, error), but the actual controller signatures show unnamed bool returns in methods like:
CreateTransaction(ctx context.Context, parameters Parameters[CreateTransaction]) (*ledger.Log, *ledger.CreatedTransaction, bool, error)SaveTransactionMetadata(ctx context.Context, parameters Parameters[SaveTransactionMetadata]) (*ledger.Log, bool, error)The
-typedflag is correctly added and beneficial for these multi-return-value methods, enabling type-safe per-call return value expectations in tests. The implementation is sound; only the variable naming description in the comment is inaccurate.internal/api/v1/controllers_accounts_add_metadata.go (1)
38-48: Correct implementation of idempotency-hit header.The implementation correctly unpacks the idempotency-hit value and sets the response header when appropriate. This matches the pattern used in other endpoints.
pkg/client/models/operations/v2addmetadatatoaccount.go (1)
63-78: Correct implementation of headers exposure.The addition of the Headers field and GetHeaders method correctly exposes response headers to API clients. The nil check in GetHeaders prevents potential panics.
internal/api/v1/controllers_transactions_add_metadata.go (1)
33-48: Correct implementation of idempotency-hit header.The implementation correctly unpacks the idempotency-hit value and sets the response header when appropriate. The error handling remains unchanged and correct.
internal/api/v2/controllers_accounts_delete_metadata.go (1)
22-37: LGTM!The idempotency handling is correctly implemented:
- The additional
idempotencyHitreturn value is properly unpacked- The
Idempotency-Hitheader is conditionally set when true- Header is set before writing the response body
internal/api/v2/controllers_accounts_add_metadata.go (1)
26-36: LGTM!The idempotency handling follows the same correct pattern as other endpoints.
docs/api/README.md (1)
975-979: LGTM!The API documentation properly documents the
Idempotency-Hitresponse header andIdempotency-Keyrequest header across all relevant endpoints.internal/api/v2/controllers_transactions_revert.go (1)
39-63: LGTM!The idempotency handling is correctly implemented, following the same pattern as other endpoints.
internal/api/v1/controllers_transactions_revert.go (1)
25-48: LGTM!The idempotency handling is correctly implemented. The previous issue flagged in review comments has been properly addressed.
internal/api/v2/controllers_transactions_add_metadata.go (1)
27-42: LGTM!The idempotency handling follows the correct pattern consistently applied across all endpoints.
internal/api/v2/controllers_transactions_delete_metadata.go (1)
26-38: LGTM!The idempotency handling is correctly implemented. The header is set only on the success path after error handling, and
w.Header().Set()is called beforeapi.NoContent(w), ensuring the header is properly included in the response.internal/api/v1/controllers_transactions_create.go (2)
87-114: LGTM!The idempotency handling for the postings-based transaction creation path is correctly implemented. The header is set after successful execution and before the response is sent.
123-152: LGTM!The script-based transaction creation path mirrors the postings-based path with consistent idempotency handling. Both branches are properly updated.
internal/api/v1/controllers_transactions_delete_metadata.go (1)
27-39: LGTM!The idempotency handling is now correctly implemented, consistent with the v2 controller and other v1 endpoints. The previously flagged issue has been properly addressed.
internal/controller/ledger/controller_with_too_many_client_handling_test.go (2)
31-52: LGTM!The mock expectations and test assertions are correctly updated to handle the additional
idempotencyHitreturn value. The mock returnsfalseappropriately since retry scenarios aren't idempotency hits.
65-81: LGTM!The "finally failing" test case is correctly updated with the same pattern. The mock expectations properly return
falseforidempotencyHitin error scenarios.internal/controller/ledger/log_process.go (1)
89-133: LGTM!The
forgeLogmethod correctly signals idempotency hits:
- Returns
truewhen an existing log is reused via idempotency key lookup (lines 101, 126)- Returns
falsefor fresh log creation (line 132) or error paths (lines 98, 120, 128)The boolean semantics are clear and the implementation properly distinguishes between reusing a cached result versus creating a new one.
internal/controller/ledger/controller_with_too_many_client_handling_generated_test.go (1)
1-77: LGTM - Generated mock file with typed wrappers.This is an auto-generated file with the
-typedflag for mockgen, which correctly produces per-call wrapper types for type-safe mock configuration. The pattern is consistent with other mock files in the PR.Note: The
-typedflag appears twice in the generate command (line 5), which is redundant but harmless.internal/api/v2/controllers_bulk_test.go (9)
60-106: LGTM - Mock expectations correctly updated for new return signature.The test expectations are properly updated to include the
idempotencyHitboolean (false) before the error return value, consistent with the updatedCreateTransactionmethod signature.
119-136: LGTM - SaveTransactionMetadata mock updated correctly.
149-166: LGTM - SaveAccountMetadata mock updated correctly.
178-216: LGTM - RevertTransaction mock updated correctly.
227-242: LGTM - DeleteTransactionMetadata mock updated correctly.
277-312: LGTM - Error scenario mocks correctly include the idempotencyHit boolean.The error test cases properly return
falseforidempotencyHitalongside the error, maintaining consistency with the success cases.
351-396: LGTM - Continue-on-failure test case mocks updated correctly.
425-465: LGTM - Atomic transaction test mocks updated correctly.
514-558: LGTM - Custom content type test mock updated correctly.internal/controller/ledger/controller_with_traces.go (6)
365-387: LGTM - CreateTransaction correctly propagates idempotencyHit through tracing.The implementation properly captures
idempotencyHitfrom the underlying controller call and propagates it through the return, withfalsereturned on error paths.
389-411: LGTM - RevertTransaction correctly propagates idempotencyHit through tracing.
413-433: LGTM - SaveTransactionMetadata correctly propagates idempotencyHit through tracing.
435-456: LGTM - SaveAccountMetadata correctly propagates idempotencyHit through tracing.
458-479: LGTM - DeleteTransactionMetadata correctly propagates idempotencyHit through tracing.
481-502: LGTM - DeleteAccountMetadata correctly propagates idempotencyHit through tracing.internal/controller/ledger/controller_with_events.go (6)
43-55: LGTM - CreateTransaction correctly propagates idempotencyHit through events wrapper.The implementation captures and propagates
idempotencyHitwhile preserving existing event listener behavior.
57-74: LGTM - RevertTransaction correctly propagates idempotencyHit through events wrapper.
76-94: LGTM - SaveTransactionMetadata correctly propagates idempotencyHit through events wrapper.
96-114: Consider: Should events be suppressed on idempotent hits?Events are currently fired regardless of
idempotencyHitstatus. If an operation returns cached results due to idempotency, downstream consumers may receive duplicate events for the same logical operation.If this is intentional (e.g., for at-least-once delivery semantics), please disregard. Otherwise, consider guarding event firing with
!idempotencyHit.
116-134: LGTM - DeleteTransactionMetadata correctly propagates idempotencyHit through events wrapper.
136-154: LGTM - DeleteAccountMetadata correctly propagates idempotencyHit through events wrapper.internal/controller/ledger/controller.go (2)
19-19: LGTM - Mock generation updated with-typedflag.The mockgen directive correctly includes the
-typedflag for generating type-safe mock wrappers.
53-75: LGTM - Remaining method signatures correctly updated.All mutating operations that can leverage idempotency (
RevertTransaction,SaveTransactionMetadata,SaveAccountMetadata,DeleteTransactionMetadata,DeleteAccountMetadata) now return theidempotencyHitboolean. The signature changes are consistent and idiomatic.internal/controller/system/state_tracker.go (1)
114-197: Idempotency flag is cleanly forwarded through the state-tracking facadeThe new signatures and closure pattern correctly propagate
idempotencyHitfrom the underlying controller throughhandleState, without altering the existing locking/transaction behavior. Looks good.internal/controller/ledger/controller_default.go (1)
415-565: DefaultController correctly surfaces idempotencyHit from log processingCreate/Revert and the Save*/Delete* metadata methods now consistently return the extra
idempotencyHitflag fromlogProcessor.forgeLog, while preserving previous return shapes and behavior. The separation between the internal Saved/Deleted payload and the public API remains clear and correct.internal/api/common/mocks_ledger_controller_test.go (1)
109-428: LedgerController common mocks match updated controller signaturesThe generated mocks correctly add the
boolreturn forCreateTransaction,RevertTransaction, and the Save*/Delete* metadata methods, preserving order and types. This keeps tests aligned with the new idempotency-aware interface.internal/api/v2/mocks_system_controller_test.go (1)
5-1299: Typed gomock wrappers for v2 system mocks look consistent and type-safeThe new per-method
*...Callwrappers and-typedgeneration integrate cleanly with existing mocks, improving type safety on expectations without altering runtime behavior. No issues spotted.internal/api/v1/mocks_system_controller_test.go (1)
5-1299: v1 SystemController typed mocks correctly wrap gomock.CallRecorder methods now returning typed
SystemController*Callwrappers are consistent with the interface and other mocks, and the added Return/Do/DoAndReturn helpers are well-typed. Generated code looks fine.internal/controller/ledger/controller_with_too_many_client_handling.go (1)
46-129: Retry wrapper now correctly forwards idempotencyHit from the underlying controllerThe TooManyClient-handling wrapper methods simply add an
idempotencyHitlocal, invoke the underlying controller viahandleRetry, and return the extended tuple. This keeps retry semantics unchanged while exposing the new flag. Implementation looks solid.internal/api/v1/mocks_ledger_controller_test.go (1)
5-1028: v1 LedgerController mocks align with the new idempotency-aware interfaceThe generated mocks correctly add the
boolreturn for transactional and metadata operations and wrap recorder calls in typedLedgerController*Callhelpers. Types and ordering all match the controller interface; nothing problematic stands out.internal/api/bulking/mocks_ledger_controller_test.go (1)
5-1028: Bulking LedgerController typed mocks correctly reflect extended return valuesThese gomock-generated bulking mocks now expose the extra
boolreturn for idempotent operations and use typedLedgerController*Callwrappers, in line with the main controller interface and other mock sets. No issues detected.internal/api/v2/mocks_ledger_controller_test.go (2)
5-5: LGTM: Typed mock generation enabled.The addition of the
-typedflag to mockgen is a good improvement. This generates type-safe wrapper types for each mocked method, providing better compile-time safety and IDE support compared to generic*gomock.Callreturns.
206-213: LGTM: Mock signatures correctly reflect interface changes.The generated mocks correctly include the new
boolreturn parameter (idempotencyHit) for methods that now support idempotency tracking. The signatures match the Controller interface definition shown in the relevant code snippets.Also applies to: 247-253, 287-293, 872-879, 951-957, 991-997
internal/controller/ledger/controller_generated_test.go (2)
5-5: LGTM: Typed mock generation enabled.Consistent with the v2 API mocks, this file now uses typed mock generation for better type safety and developer experience.
205-212: LGTM: Mock signatures correctly reflect interface changes.The generated mocks correctly include the new
boolreturn parameter for idempotency tracking, matching the Controller interface. The changes are consistent with the v2 API mocks.Also applies to: 246-252, 286-292, 871-878, 950-956, 990-996
| ) | ||
|
|
||
| //go:generate mockgen -write_source_comment=false -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed | ||
| //go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed |
There was a problem hiding this comment.
Fix duplicated -typed flag in go:generate directive
The go:generate line ends with an extra -typed after . DelayCalculator, so mockgen will likely treat it as a positional argument rather than an option and may fail when running go generate.
Consider dropping the trailing -typed so the command matches the other mockgen invocations:
-//go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed
+//go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator📝 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.
| //go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator -typed | |
| //go:generate mockgen -write_source_comment=false -typed -write_package_comment=false -source controller_with_too_many_client_handling.go -destination controller_with_too_many_client_handling_generated_test.go -package ledger . DelayCalculator |
🤖 Prompt for AI Agents
In internal/controller/ledger/controller_with_too_many_client_handling.go around
line 18, the go:generate directive contains a duplicated trailing "-typed" flag
which will be treated as a positional argument by mockgen; remove the extra
"-typed" at the end so the directive matches other mockgen invocations (keep the
initial -typed flag before -write_package_comment=false) and ensure the rest of
the flags/order remain unchanged.
| | `HTTPMeta` | [components.HTTPMetadata](../../models/components/httpmetadata.md) | :heavy_check_mark: | N/A | | ||
| | `V2CreateTransactionResponse` | [*components.V2CreateTransactionResponse](../../models/components/v2createtransactionresponse.md) | :heavy_minus_sign: | OK | | ||
| | `V2CreateTransactionResponse` | [*components.V2CreateTransactionResponse](../../models/components/v2createtransactionresponse.md) | :heavy_minus_sign: | OK | | ||
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | |
There was a problem hiding this comment.
Fix markdown formatting for Headers type.
The Headers type contains a stray trailing asterisk (map[string][]*string*) that triggers markdown linting errors. Wrap the type in backticks and remove the trailing asterisk: `map[string][]*string`.
Apply this diff:
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A |
+| `Headers` | `map[string][]*string` | :heavy_check_mark: | N/A |🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
🤖 Prompt for AI Agents
In pkg/client/docs/models/operations/v2createtransactionresponse.md around line
10, the Headers type is incorrectly formatted as map[string][]*string* (with a
stray trailing asterisk) which breaks markdown linting; change it to a properly
backticked code span and remove the trailing asterisk so it reads
`map[string][]*string`.
| | `HTTPMeta` | [components.HTTPMetadata](../../models/components/httpmetadata.md) | :heavy_check_mark: | N/A | | ||
| | `V2CreateTransactionResponse` | [*components.V2CreateTransactionResponse](../../models/components/v2createtransactionresponse.md) | :heavy_minus_sign: | OK | | ||
| | `V2CreateTransactionResponse` | [*components.V2CreateTransactionResponse](../../models/components/v2createtransactionresponse.md) | :heavy_minus_sign: | OK | | ||
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | |
There was a problem hiding this comment.
Fix markdown formatting for Headers type.
The Headers type contains a stray trailing asterisk (map[string][]*string*) that triggers markdown linting errors. Wrap the type in backticks and remove the trailing asterisk: `map[string][]*string`.
Apply this diff:
-| `Headers` | map[string][]*string* | :heavy_check_mark: | N/A |
+| `Headers` | `map[string][]*string` | :heavy_check_mark: | N/A |📝 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.
| | `Headers` | map[string][]*string* | :heavy_check_mark: | N/A | | |
| | `Headers` | `map[string][]*string` | :heavy_check_mark: | N/A | |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
10-10: Reference links and images should use a label that is defined
Missing link or image reference definition: "string"
(MD052, reference-links-images)
🤖 Prompt for AI Agents
In pkg/client/docs/models/operations/v2reverttransactionresponse.md around line
10, the Headers type is incorrectly formatted as map[string][]*string* which
contains a stray trailing asterisk and breaks markdown; change it to a properly
backticked type `map[string][]*string` by removing the trailing asterisk and
wrapping the entire type in backticks so the markdown renders and lints
correctly.
