feat: allow marshalling of big ints in string using a header #959
Conversation
|
Warning Rate limit exceeded@gfyrag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits 8000 than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (16)
WalkthroughThis update introduces a new rendering layer for API responses, enabling big integers to be serialized as strings when requested via an HTTP header. Several controller handlers now use these rendering functions to transform ledger entities before responding. Supporting methods and types are added to the ledger domain, and related documentation and tests are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Controller
participant Renderer
participant Ledger
Client->>API Controller: HTTP Request (with/without Formance-Bigint-As-String header)
API Controller->>Ledger: Fetch ledger entity (e.g., transaction, account)
Ledger-->>API Controller: Return entity
API Controller->>Renderer: Call render function (entity, request)
Renderer-->>API Controller: Return rendered entity (big ints as string if header set)
API Controller-->>Client: HTTP Response (JSON, big ints as string or number)
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
internal/api/v2/controllers_accounts_read.go (1)
45-45: Avoid unnecessary struct copy
renderAccount(r, *acc)copies the wholeAccountstruct (potentially large slices & big.Ints).
Passing the pointer keeps GC pressure lower:- api.Ok(w, renderAccount(r, *acc)) + api.Ok(w, renderAccount(r, acc))(adjust
renderAccountsignature accordingly).
Not blocking, but worth considering.internal/api/v2/controllers_transactions_read.go (1)
45-45: Same copy concern as in account handler
renderTransaction(r, *tx)copies the whole transaction, including postings & volumes maps.
IfrenderTransactiondoesn’t need value semantics, prefer passing the pointer.internal/api/v2/controllers_transactions_revert.go (1)
47-47: Consider pointer to avoid copying reverted transaction
renderTransaction(r, ret.RevertTransaction)currently copies the struct.
Pass the pointer if mutability isn’t required, matching the other handlers.internal/api/v2/controllers_logs_list.go (1)
34-36: Extract mapped cursor into a variable for readability
Inliningbunpaginate.MapCursorinside theRenderCursorcall forces an extra pointer dereference and makes the control-flow harder to scan.- api.RenderCursor(w, *bunpaginate.MapCursor(cursor, func(log ledger.Log) any { - return renderLog(r, log) - })) + mapped := bunpaginate.MapCursor(cursor, func(log ledger.Log) any { + return renderLog(r, log) + }) + api.RenderCursor(w, *mapped)internal/api/v2/controllers_transactions_list.go (1)
45-47: Mirror the cursor-mapping refactor for consistencySame micro-readability win as in
controllers_logs_list.go; consider the short variable extraction pattern:- api.RenderCursor(w, *bunpaginate.MapCursor(cursor, func(tx ledger.Transaction) any { - return renderTransaction(r, tx) - })) + mapped := bunpaginate.MapCursor(cursor, func(tx ledger.Transaction) any { + return renderTransaction(r, tx) + }) + api.RenderCursor(w, *mapped)internal/api/v2/controllers_volumes.go (1)
66-68: Apply the same small refactor for symmetry with other handlers- api.RenderCursor(w, *bunpaginate.MapCursor(cursor, func(volumes ledger.VolumesWithBalanceByAssetByAccount) any { - return renderVolumesWithBalances(r, volumes) - })) + mapped := bunpaginate.MapCursor(cursor, func(v ledger.VolumesWithBalanceByAssetByAccount) any { + return renderVolumesWithBalances(r, v) + }) + api.RenderCursor(w, *mapped)internal/transaction.go (1)
208-212: Receiver should be pointer for a fluent builder
WithPostCommitVolumescopies the wholeTransactionstruct. That’s consistent with the existing builder methods, but note that the newPostCommitVolumesmap can be large – every call copies it. If you plan to chain many setters, consider using a pointer receiver to avoid unintended copies.internal/log.go (1)
98-102:WithDatecopies the value; was that intended?
Unlike most “mutator” helpers,WithDateoperates on a copy ofLog, so:log := existing.WithDate(t) // original unchangedIf the goal is mutation in place, use a pointer receiver; otherwise document that this is purely functional.
internal/api/v2/views_test.go (3)
15-60: Use deterministic timestamps for reproducible tests
now := time.Now()can introduce flakiness across slow CI machines (nanosecond drift between value capture and JSON marshal). Prefer a fixedtime.Date(...)literal.
66-152: Validate a “large” bigint scenario
All numeric-mode expectations use100, which fits safely into afloat64. Add a case using a value > 2^53 to ensure the"bigint as number"path is still precise enough or fails obviously – that is the whole point of the new header.
243-261: Helper could DRY out repeated render/assert boilerplate
Every test block repeats the request creation, marshal/unmarshal, and map comparison. A small helper (e.g.assertRender(t, headers, expected, func(*http.Request) any)) would shrink this file by ~40 %.internal/api/v2/views.go (1)
218-221: Consider using a switch statement for better readability.While the current implementation works correctly, a switch statement would be more idiomatic and readable.
Apply this diff to improve readability:
-func needBigIntAsString(r *http.Request) bool { - v := strings.ToLower(r.Header.Get(HeaderBigIntAsString)) - return v == "true" || v == "yes" || v == "y" || v == "1" -} +func needBigIntAsString(r *http.Request) bool { + switch strings.ToLower(r.Header.Get(HeaderBigIntAsString)) { + case "true", "yes", "y", "1": + return true + default: + return false + } +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
internal/README.md(51 hunks)internal/api/v2/controllers_accounts_list.go(2 hunks)internal/api/v2/controllers_accounts_read.go(1 hunks)internal/api/v2/controllers_balances.go(1 hunks)internal/api/v2/controllers_logs_list.go(2 hunks)internal/api/v2/controllers_transactions_create.go(2 hunks)internal/api/v2/controllers_transactions_list.go(2 hunks)internal/api/v2/controllers_transactions_read.go(1 hunks)internal/api/v2/controllers_transactions_revert.go(1 hunks)internal/api/v2/controllers_volumes.go(2 hunks)internal/api/v2/views.go(1 hunks)internal/api/v2/views_test.go(1 hunks)internal/controller/ledger/controller_default_test.go(1 hunks)internal/log.go(6 hunks)internal/transaction.go(2 hunks)internal/volumes.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
internal/api/v2/controllers_transactions_revert.go (1)
internal/controller/ledger/controller.go (1)
RevertTransaction(99-103)
internal/api/v2/controllers_transactions_list.go (1)
internal/transaction.go (2)
Transaction(37-49)Transaction(51-58)
internal/api/v2/controllers_logs_list.go (1)
internal/log.go (1)
Log(84-96)
internal/api/v2/controllers_volumes.go (1)
internal/volumes.go (1)
VolumesWithBalanceByAssetByAccount(63-67)
internal/log.go (1)
internal/storage/ledger/logs.go (1)
Log(20-26)
internal/api/v2/views.go (5)
internal/volumes.go (6)
Volumes(12-15)Volumes(40-43)VolumesByAssets(98-98)PostCommitVolumes(118-118)VolumesWithBalanceByAssetByAccount(63-67)BalancesByAssets(96-96)internal/transaction.go (2)
Transaction(37-49)Transaction(51-58)internal/posting.go (1)
Postings(35-35)internal/account.go (1)
Account(14-24)internal/log.go (5)
CreatedTransaction(199-202)RevertedTransaction(329-332)Log(84-96)NewTransactionLogType(22-22)RevertedTransactionLogType(23-23)
🪛 LanguageTool
internal/README.md
[duplication] ~74-~74: Possible typo: you repeated a word.
Context: ...a PostCommitVolumes) SubtractPostings(postings Postings) PostCommitVolumes](<#PostCommitVolume...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (34)
internal/api/v2/controllers_balances.go (1)
37-37: LGTM – consistent with new rendering layerUsing
renderBalancesByAssetskeeps API output homogeneous with the bigint-stringification feature.internal/controller/ledger/controller_default_test.go (1)
64-65: Constant update looks correct
Expectation now matches the renamed domain constantNewTransactionLogType. Test semantics stay intact.internal/api/v2/controllers_logs_list.go (1)
7-8: Import alias justified
The newledgerimport is required for the generic type argument below—no issues.internal/api/v2/controllers_volumes.go (1)
6-8: Added imports are required
bunpaginateand theledgeralias are now used after mapping. Looks good.internal/api/v2/controllers_transactions_create.go (2)
6-7: Import re-ordering is gofmt-compliant
No functional impact.
71-72: Output now passes through renderer – good alignment
Switching torenderTransactionguarantees consistent big-int handling across endpoints. No issues spotted.internal/api/v2/controllers_accounts_list.go (1)
6-8: Imports look good – ledger alias is now required
The addedbunpaginateandledgerimports are both used below, satisfying the compiler and aligning this handler with the other v2 controllers.internal/transaction.go (1)
182-197: Nil-map safety when subtracting postings
tx.PostCommitVolumes(amap) may legally benil. Calling a method that iterates over a nil map is fine, but ifSubtractPostingsitself assumes a non-nil receiver, you will hit a panic. Double-check the implementation or guard here.internal/log.go (3)
22-23: Renamed constant – confirm DB/back-compat
Behaviour stays identical, but double-check any migrations/seed data that referencedNEW_LOG_TYPEor relied on the previous enum ordering.
205-206: Consistency achieved withNewTransactionLogType
TheCreatedTransaction.Type()switch-case now matches the renamed constant. Good catch.
368-372:HydrateLogswitch updated – no other code path missing
The new case guarantees proper unmarshalling of"NEW_TRANSACTION"logs. Looks complete.internal/api/v2/views.go (4)
1-10: LGTM!Clean and well-organized imports.
12-12: LGTM!Well-named constant following Go conventions.
14-26: LGTM!Clean implementation of MarshalJSON for volumes with proper string conversion of big integers.
79-84:✅ Verification successful
Verify the SubtractPostings method exists on PostCommitVolumes.
The code calls
SubtractPostingson PostCommitVolumes to compute pre-commit volumes. Ensure this method is properly implemented.
🏁 Script executed:
#!/bin/bash # Description: Verify that SubtractPostings method exists on PostCommitVolumes type # Search for SubtractPostings method definition ast-grep --pattern 'func ($_ PostCommitVolumes) SubtractPostings($_) $_'Length of output: 734
No action needed – SubtractPostings method exists
The
SubtractPostingsmethod is defined onPostCommitVolumesininternal/volumes.go(starting at line 120), so the calls ininternal/api/v2/views.goare valid.internal/README.md (19)
53-53: Approve new index entry for Log.WithDate
The documentation now includes theLog.WithDate(date time.Time) Logmethod, matching the implementation ininternal/log.go.
253-253: Approve addition of AccountMetadata type
The newAccountMetadata map[string]metadata.Metadatatype is correctly documented and aligns with its declaration ininternal/log.go.
278-278: Approve addition of AggregatedVolumes type
TheAggregatedVolumestype is properly listed and reflects the structure ininternal/volumes.go.
347-347: Approve addition of CreatedTransaction type
TheCreatedTransactiontype entry is correctly documented, matching its definition ininternal/log.go.
359-359: Approve addition of CreatedTransaction.GetMemento
TheGetMemento()method entry aligns with the implementation ininternal/log.go.
368-368: Approve addition of CreatedTransaction.Type
TheType() LogTypemethod is documented as expected and matches its code ininternal/log.go.
377-377: Approve addition of DeletedMetadata type
TheDeletedMetadatatype entry is correct and consistent with the code ininternal/log.go.
390-390: Approve addition of DeletedMetadata.Type
TheType() LogTypemethod forDeletedMetadatais accurately documented.
399-399: Approve addition of DeletedMetadata.UnmarshalJSON
TheUnmarshalJSON(data []byte) errorsignature is correctly reflected in the docs.
549-549: Approve addition of NewLog factory function
TheNewLog(payload LogPayload) Logentry matches the new constructor ininternal/log.go.
558-558: Approve addition of Log.ChainLog
TheChainLog(previous *Log) Logmethod is now documented and aligns with its implementation.
567-567: Approve addition of Log.ComputeHash
TheComputeHash(previous *Log)entry correctly matches the code change.
576-576: Approve addition of Log.UnmarshalJSON
The pointer receiverUnmarshalJSON(data []byte) errormethod is accurately included.
584-589: Approve full doc entry for Log.WithDate
Both the anchor and method signature forWithDate(date time.Time) Logare present and correct.
645-645: Approve rename to NewTransactionLogType
The constantNewTransactionLogTypereflects the code rename fromNewLogTypeininternal/log.go.
800-805: Approve addition of PostCommitVolumes.SubtractPostings
TheSubtractPostings(postings Postings) PostCommitVolumesmethod is correctly documented and matches the new implementation.
1067-1072: Approve addition of Transaction.WithPostCommitVolumes
The newWithPostCommitVolumes(volumes PostCommitVolumes) Transactionmethod is properly listed and consistent withinternal/transaction.go.
1128-1132: Approve addition of NewTransactionData
TheNewTransactionData() TransactionDataconstructor is now included in the index as expected.
1137-1141: Approve addition of TransactionData.WithPostings
TheWithPostings(postings ...Posting) TransactionDatamethod entry correctly mirrors the code.
| func (a PostCommitVolumes) SubtractPostings(postings Postings) PostCommitVolumes { | ||
| if len(a) == 0 { | ||
| return PostCommitVolumes{} | ||
| } | ||
| ret := a.Copy() | ||
| for _, posting := range postings { | ||
| ret.AddOutput(posting.Source, posting.Asset, big.NewInt(0).Neg(posting.Amount)) | ||
| ret.AddInput(posting.Destination, posting.Asset, big.NewInt(0).Neg(posting.Amount)) | ||
| } | ||
|
|
||
| return ret | ||
| } |
There was a problem hiding this comment.
Potential panic when source/destination account or asset is missing
AddInput/AddOutput assume that a[account] and a[account][asset] are already initialised.
If SubtractPostings is called with a posting whose source/destination didn’t exist in the original PostCommitVolumes, the index operation on a nil map will dereference a nil value and panic at runtime.
volumes := a[account][asset].Copy() // ← panics when a[account] == nilRecommended fix: lazily initialise the nested maps before accessing them.
func (a PostCommitVolumes) AddInput(account, asset string, input *big.Int) {
- volumes := a[account][asset].Copy()
+ if _, ok := a[account]; !ok {
+ a[account] = VolumesByAssets{}
+ }
+ if _, ok := a[account][asset]; !ok {
+ a[account][asset] = NewEmptyVolumes()
+ }
+ volumes := a[account][asset].Copy()
volumes.Input.Add(volumes.Input, input)
a[account][asset] = volumes
}Do the same for AddOutput.
Minor perf-nit: big.NewInt(0).Neg(posting.Amount) allocates each loop; reuse a scratch variable to avoid N allocations on large posting slices.
🤖 Prompt for AI Agents
In internal/volumes.go around lines 120 to 131, the SubtractPostings method can
panic because AddInput and AddOutput assume nested maps are initialized, but
they may be nil for new accounts or assets. Fix this by lazily initializing the
nested maps inside AddInput and AddOutput before accessing them. Also, optimize
the loop by creating a single big.Int variable outside the loop to hold the
negated amount and reuse it for each posting instead of allocating a new big.Int
every iteration.
There was a problem hiding this comment.
If PostCommitVolumes is not empty, it must contains required data.
We don't want to ignore the errors if it does not contains the required data.
There was a problem hiding this comment.
@gfyrag Got it—if the surrounding logic guarantees that every (account, asset) pair referenced by a posting is already present whenever PostCommitVolumes is non-empty, leaving the current implementation as-is is perfectly reasonable.
You might still consider one of the following (purely optional) hardening steps:
• Add an explicit check with a descriptive panic/log.Fatalf so that, if the invariant is ever violated, you get a clearer message than “invalid memory address or nil pointer dereference”.
• Add a short comment above AddInput/AddOutput documenting the pre-condition (“volume entries must already exist”).
Both keep the fail-fast behavior you want while improving debuggability, but I’m fine if you prefer the current minimal version.
| api.RenderCursor(w, *bunpaginate.MapCursor(cursor, func(account ledger.Account) any { | ||
| return renderAccount(r, account) | ||
| })) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential panic if bunpaginate.MapCursor returns nil
MapCursor returns *bunpaginate.Cursor.
api.RenderCursor(w, *bunpaginate.MapCursor(...))If, for any reason, MapCursor returns nil, dereferencing it will panic. A defensive check (or keeping the pointer and letting RenderCursor accept it) would eliminate this foot-gun.
- api.RenderCursor(w, *bunpaginate.MapCursor(cursor, func(account ledger.Account) any {
- return renderAccount(r, account)
- }))
+ mapped := bunpaginate.MapCursor(cursor, func(account ledger.Account) any {
+ return renderAccount(r, account)
+ })
+ if mapped == nil {
+ api.BadRequest(w, common.ErrValidation, errors.New("failed to map cursor"))
+ return
+ }
+ api.RenderCursor(w, *mapped)🤖 Prompt for AI Agents
In internal/api/v2/controllers_accounts_list.go around lines 35 to 37, the code
dereferences the pointer returned by bunpaginate.MapCursor without checking if
it is nil, which can cause a panic. Modify the code to first assign the result
of MapCursor to a variable, check if it is nil, and handle that case
appropriately before dereferencing. Alternatively, adjust RenderCursor to accept
a pointer and pass the pointer directly without dereferencing.
There was a problem hiding this comment.
MapCursor return a cursor, the method should return a value, and not a pointer. It should be fixed on the library.
There was a problem hiding this comment.
@gfyrag Understood—thanks for clarifying. If MapCursor is intended to return a value (not a *Cursor), then the ideally safe fix belongs in bunpaginate itself so every caller benefits and we can keep the concise dereference-free call here. I’ll drop the earlier suggestion and leave this handler as-is.
Let me know if you’d like help opening an issue or PR on go-libs to adjust the return type; otherwise I’m all set. 🐇
There was a problem hiding this comment.
Seems legit, you can open a pr.
There was a problem hiding this comment.
A summary of the edits CodeRabbit can apply:
I will open a PR in the
go-libsrepository (not in this repo) that changesbunpaginate.MapCursorto return a non-pointerCursorvalue so callers no longer need to dereference it. No change is needed in the current file.
- ✅ Create PR with these edits
- 📌 Create commit in current branch
- 📋 Get copyable edits
| switch l.Type { | ||
| case ledger.NewTransactionLogType: | ||
| return createdTransaction(l.Data.(ledger.CreatedTransaction)) | ||
| case ledger.RevertedTransactionLogType: | ||
| return revertedTransaction(l.Data.(ledger.RevertedTransaction)) | ||
| default: | ||
| return l.Data | ||
| } |
There was a problem hiding this comment.
Add safety checks for type assertions to prevent panics.
The type assertions on lines 200 and 202 don't check the second return value, which could cause panics if the assertions fail.
Apply this diff to add safety checks:
- switch l.Type {
- case ledger.NewTransactionLogType:
- return createdTransaction(l.Data.(ledger.CreatedTransaction))
- case ledger.RevertedTransactionLogType:
- return revertedTransaction(l.Data.(ledger.RevertedTransaction))
- default:
- return l.Data
- }
+ switch l.Type {
+ case ledger.NewTransactionLogType:
+ if ct, ok := l.Data.(ledger.CreatedTransaction); ok {
+ return createdTransaction(ct)
+ }
+ return l.Data
+ case ledger.RevertedTransactionLogType:
+ if rt, ok := l.Data.(ledger.RevertedTransaction); ok {
+ return revertedTransaction(rt)
+ }
+ return l.Data
+ default:
+ return l.Data
+ }📝 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.
| switch l.Type { | |
| case ledger.NewTransactionLogType: | |
| return createdTransaction(l.Data.(ledger.CreatedTransaction)) | |
| case ledger.RevertedTransactionLogType: | |
| return revertedTransaction(l.Data.(ledger.RevertedTransaction)) | |
| default: | |
| return l.Data | |
| } | |
| switch l.Type { | |
| case ledger.NewTransactionLogType: | |
| if ct, ok := l.Data.(ledger.CreatedTransaction); ok { | |
| return createdTransaction(ct) | |
| } | |
| return l.Data | |
| case ledger.RevertedTransactionLogType: | |
| if rt, ok := l.Data.(ledger.RevertedTransaction); ok { | |
| return revertedTransaction(rt) | |
| } | |
| return l.Data | |
| default: | |
| return l.Data | |
| } |
🤖 Prompt for AI Agents
In internal/api/v2/views.go around lines 198 to 205, the type assertions on
l.Data for ledger.CreatedTransaction and ledger.RevertedTransaction lack safety
checks, risking panics if the assertion fails. Modify the code to use the
two-value form of type assertion, check the boolean result, and handle the
failure case gracefully, such as returning an error or a default value, to
prevent runtime panics.
There was a problem hiding this comment.
We don't want to ignore a conversion error. The types should be these types. If not, we are ok with the fact than the code will panic, as it would be a development error.
There was a problem hiding this comment.
@gfyrag Got it—thanks for clarifying. I’ll keep in mind that, for these internal assertions, a panic is the intended behaviour when the data doesn’t match the expected type.
(⁎ᵕᴗᵕ⁎)🪃
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #959 +/- ##
==========================================
+ Coverage 82.43% 82.82% +0.38%
==========================================
Files 141 142 +1
Lines 7886 8028 +142
==========================================
+ Hits 6501 6649 +148
+ Misses 1059 1054 -5
+ Partials 326 325 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
30f395c to
15ea8a4
Compare
15ea8a4 to
af20ac1
Compare
Fixes LX-54