-
Couldn't load subscription status.
- Fork 130
feat: handle missing compatibility layer with v2.1 #634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes introduce new paginated resource adapters for various ledger entities in the legacy storage system. These adapters provide methods for retrieving, counting, and paginating accounts, logs, transactions, aggregated balances, and volumes. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/storage/ledger/legacy/adapters.go (4)
76-79: Panic usage in Log adapter methods
BothGetOneandCountpanic here. Consider returning an error instead of panicking to avoid crashing the service if these methods are accidentally invoked.Also applies to: 80-82
157-181: Big integer usage
The method starts each output balance atnew(big.Int)and usesAdd. A directSet(balance)could be more explicit, e.g.,Input = new(big.Int).Set(balance). Otherwise, logic looks solid for constructing aggregated volumes.
183-185: Panic in Count
Again, consider returning an appropriate error rather than panicking, to safeguard runtime stability.
208-210: Panic in GetOne and Count
Returning an error is typically safer than panicking for unimplemented methods.Also applies to: 212-214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/storage/ledger/legacy/adapters.go(1 hunks)
🔇 Additional comments (20)
internal/storage/ledger/legacy/adapters.go (20)
6-6: No concerns on new imports
The newly introduced imports (bunpaginate,math/big, andslices) appear consistent with the newly added pagination and big integer functionalities.Also applies to: 14-15
18-20: Struct definition is straightforward
TheaccountsPaginatedResourceAdapterwith a*Storepointer looks appropriate for the intended usage.
22-39: Type assertion safety
In theGetOnemethod, the code assertsvalue.(string)at line 25. Ifquery.Builderunexpectedly yields a non-string value, it may cause a runtime panic. Consider handling or validating the type to guard against malformed queries.
41-53: Counting accounts with expansions
Though expansions like volumes or effective volumes generally do not affect record count, the code passes these flags into the store query. This is typically fine, but ensure that expansions don’t unintentionally impact performance or logic in the count routine.
55-68: Pagination logic
The code properly usesGetAccountsWithVolumeswith page-sized queries. Ensure that the underlying store handles edge cases such as zero or negative page sizes.
70-70: Compile-time interface check
This line confirms thataccountsPaginatedResourceAdapterimplements thePaginatedResourceinterface. No issues.
72-74: Struct definition
ThelogsPaginatedResourceAdapterwith astorefield maintains a consistent approach.
84-95: Logs pagination
Usingp.store.GetLogsis consistent with the rest of the pattern. No immediate concerns.
97-98: Compile-time interface check
VerifieslogsPaginatedResourceAdaptermeetsPaginatedResourcerequirements. Looks good.
99-101: Transactions adapter struct
ThetransactionsPaginatedResourceAdaptermirrors the accounts/logs adapters. The approach remains consistent.
103-120: Type assertion safety
InGetOne, the code assertsvalue.(int)at line 106. Similar to the accounts adapter, consider confirming the type if invalid input might pass through the builder.
122-134: Transactions count
Acceptable usage ofCountTransactions. Ensure expansions do not degrade performance if they’re unnecessary for counting.
136-149: Transactions pagination
Logic is consistent with the codebase's approach to pagination. No immediate concerns.
151-152: Compile-time interface check
ConfirmstransactionsPaginatedResourceAdapteraligns withPaginatedResource.
153-155: Aggregated balances adapter
Basic struct definition follows the same pattern as other adapters.
187-188: Compile-time interface check
No concerns here.
189-191: Aggregated volumes adapter
Struct definition is analogous to the others.
193-206: Aggregated volumes pagination
Logic for retrieving volumes with balances is in harmony with the existing approach.
216-217: Compile-time interface check
Ensures the aggregated volumes adapter fulfillsPaginatedResource.
225-227: Conditional adapter delegation
All these condition checks in theDefaultStoreAdapterproperly toggle between legacy and new store adapters viaisFullUpToDate. No issues found.Also applies to: 232-234, 239-241, 246-248, 253-255
No description provided.