fix: import from 2.1 #689
fix: import from 2.1 #689
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request implements significant updates across the repository. The Ledger API documentation is revised to transition from version 1 to version 2, with detailed endpoint descriptions and examples. Internally, ID fields for logs and transactions are converted from value types to pointer types, and new helper methods ( Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Router (v2)
participant Ctrl as Controller
participant Store as Ledger Store
C->>R: Sends API request (e.g. CreateTransaction)
R->>Ctrl: Forwards request with environment parameter ("develop")
Ctrl->>Store: Executes operations (e.g. InsertTransaction using dereferenced IDs)
Store-->>Ctrl: Returns transaction/log result with pointer-based IDs
Ctrl-->>R: Sends back processed data (using WithID methods)
R-->>C: Responds with updated v2 API response
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 5
🧹 Nitpick comments (11)
internal/storage/ledger/legacy/adapters.go (3)
55-68: Optional: enforce maximum page size for pagination.
If user inputs are unbounded, large page sizes could impact performance. Consider bounding or validatingquery.PageSizeto avoid potential performance issues.
136-149: Consider bounding the transactionsPaginatequery size.
Like with accounts, you may want to validate or limitquery.PageSizefor performance reasons.
157-181: Clarify usage ofbig.Intwhen constructingAggregatedVolumes.
Usingnew(big.Int).Add(new(big.Int), balance)effectively copies thebalancevalue, but it's slightly verbose. For clarity, consider a direct set:ret.Aggregated[asset] = ledger.Volumes{ - Input: new(big.Int).Add(new(big.Int), balance), + Input: new(big.Int).Set(balance), Output: new(big.Int), }pkg/client/v2.go (2)
32-219: Consider explicitly handling 4xx responses & reducing code duplication.
Error Handling for 4xx: The function currently branches for status code 200, the 5xx range, and a default case. Explicitly handling 4xx (e.g., 400, 401, 403, 404) can clarify error flow and prevent unexpected lumps of client errors under the “default” branch.
Repeated Request/Retry Logic: The block for hooking, retrying, security population, etc. reappears in multiple functions (e.g.,
GetMetrics). You could extract it as a helper to improve readability and maintainability.
223-397: Refactor repeated patterns and consider large-body retries.
Code Duplication: The request creation, retry setup, and hooking logic closely mirrors the approach in
GetInfo. Consider centralizing these patterns to avoid repeated code blocks.Potential Large Payload Overhead: Copying the request body with
GetBody()each time for retries can be expensive if future placements of metrics or other endpoints involve large payloads. You could handle small GET requests differently from large requests, or allow a one-time read/caching strategy for large bodies.internal/api/v2/controllers_transactions_read_test.go (1)
31-31: Consider using dynamic transaction ID for more robust testing.The change from using
tx.IDto a hardcoded value0makes the test less dynamic and might not effectively test real-world scenarios.Apply this diff to restore dynamic ID testing:
- Builder: query.Match("id", 0), + Builder: query.Match("id", tx.ID),internal/controller/ledger/log_process_test.go (1)
72-75: LGTM! Mock behavior updated for pointer-based IDs.The mock now correctly simulates setting the ID field using a pointer, which aligns with the new ID field type changes.
Consider extracting the ID assignment to a helper function for reuse across tests:
- DoAndReturn(func(ctx context.Context, log *ledger.Log) error { - log.ID = pointer.For(0) - return nil - }) + DoAndReturn(func(ctx context.Context, log *ledger.Log) error { + return assignLogID(log, 0) + }) +func assignLogID(log *ledger.Log, id int) error { + log.ID = pointer.For(id) + return nil +}internal/api/v2/controllers_transactions_revert_test.go (1)
84-84: LGTM! Consider using a constant for the environment string.The addition of the environment parameter aligns with the compatibility fix. However, consider defining the environment string as a constant to maintain consistency across test files.
+const testEnvironment = "develop" + func TestTransactionsRevert(t *testing.T) { // ... - router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") + router := NewRouter(systemController, auth.NewNoAuth(), testEnvironment, os.Getenv("DEBUG") == "true")internal/api/v1/routes.go (1)
32-32: LGTM! Consider adding documentation for the exported function.The change to export
GetInfoaligns with the fix for the GET /_info endpoint. Since this is now a public API, consider adding documentation to describe its purpose and usage.+// GetInfo handles the GET /_info endpoint request. +// It returns information about the system controller and version. +// Parameters: +// - systemController: The system controller instance +// - version: The API version string router.Get("/_info", GetInfo(systemController, version))internal/storage/bucket/migrations/11-make-stateless/up.sql (1)
32-45: Schema Alterations and Column Renaming in Moves Table
The migration alters themovestable by adding thetransactions_idcolumn, dropping NOT NULL constraints for certain volume columns, and renamingaccount_addresstoaccounts_address(and similarly for the address array). These changes improve consistency with updated naming conventions. Verify that all application queries and indices are adjusted accordingly.
[verified]pkg/client/docs/sdks/v2/README.md (1)
85-134:GetMetricsDocumentation Section Added
The GetMetrics section follows a similar template as GetInfo and is clear and complete. It outlines:
- A concise introduction ("Read in memory metrics")—consider hyphenating this to "Read in-memory metrics" for consistency.
- A full example usage demonstrating the client construction and endpoint call.
- Detailed tables for parameters and responses.
Ensure that the linked response type ([*operations.GetMetricsResponse]) accurately reflects the API’s output and that the anchor link (
#getmetrics) is generated correctly by your markdown renderer.
🛑 Comments failed to post (5)
internal/storage/ledger/legacy/adapters.go (5)
183-185: 🛠️ Refactor suggestion
Stop using
panicfor unimplemented methods.
Similar to the logs adapter, return an error or remove the method if truly never needed.
208-214: 🛠️ Refactor suggestion
Avoid panics in
GetOneandCountmethods.
Prevent unexpected runtime failures with a safer stub or error return.
22-39: 🛠️ Refactor suggestion
Handle potential type mismatch in
GetOneto avoid panic.
This code performsvalue.(string)without verifying thatvalueis actually astring. If the underlying type differs, it will panic.Consider returning an error instead of panicking:
func (p accountsPaginatedResourceAdapter) GetOne(ctx context.Context, query ledgercontroller.ResourceQuery[any]) (*ledger.Account, error) { var address string err := query.Builder.Walk(func(_ string, _ string, value any) error { - address = value.(string) - return nil + s, ok := value.(string) + if !ok { + return fmt.Errorf("expected string but got %T", value) + } + address = s + return nil }) if err != nil { return nil, err } return p.store.GetAccountWithVolumes(ctx, GetAccountQuery{ ... }) }📝 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.func (p accountsPaginatedResourceAdapter) GetOne(ctx context.Context, query ledgercontroller.ResourceQuery[any]) (*ledger.Account, error) { var address string err := query.Builder.Walk(func(_ string, _ string, value any) error { s, ok := value.(string) if !ok { return fmt.Errorf("expected string but got %T", value) } address = s return nil }) if err != nil { return nil, err } return p.store.GetAccountWithVolumes(ctx, GetAccountQuery{ PITFilterWithVolumes: PITFilterWithVolumes{ PITFilter: PITFilter{ PIT: query.PIT, OOT: query.OOT, }, ExpandVolumes: slices.Contains(query.Expand, "volumes"), ExpandEffectiveVolumes: slices.Contains(query.Expand, "effectiveVolumes"), }, Addr: address, }) }
72-82: 🛠️ Refactor suggestion
Avoid implementing placeholder methods that panic.
Relying onpanic("never used")can lead to unexpected runtime failures if the method is ever called. Consider returning an error or leaving them unimplemented with a clear message.func (p logsPaginatedResourceAdapter) GetOne(_ context.Context, _ ledgercontroller.ResourceQuery[any]) (*ledger.Log, error) { - panic("never used") + return nil, errors.New("GetOne for logsPaginatedResourceAdapter is not implemented") } func (p logsPaginatedResourceAdapter) Count(_ context.Context, _ ledgercontroller.ResourceQuery[any]) (int, error) { - panic("never used") + return 0, errors.New("Count for logsPaginatedResourceAdapter is not implemented") }📝 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.type logsPaginatedResourceAdapter struct { store *Store } func (p logsPaginatedResourceAdapter) GetOne(_ context.Context, _ ledgercontroller.ResourceQuery[any]) (*ledger.Log, error) { return nil, errors.New("GetOne for logsPaginatedResourceAdapter is not implemented") } func (p logsPaginatedResourceAdapter) Count(_ context.Context, _ ledgercontroller.ResourceQuery[any]) (int, error) { return 0, errors.New("Count for logsPaginatedResourceAdapter is not implemented") }
103-120: 🛠️ Refactor suggestion
Handle integer type assertion safely in
GetOne.
Similar to the accounts adapter, the code assertsvalue.(int). This will panic ifvalueis not anint.func (p transactionsPaginatedResourceAdapter) GetOne(ctx context.Context, query ledgercontroller.ResourceQuery[any]) (*ledger.Transaction, error) { var txID int err := query.Builder.Walk(func(_ string, _ string, value any) error { - txID = value.(int) - return nil + i, ok := value.(int) + if !ok { + return fmt.Errorf("expected int but got %T", value) + } + txID = i + return nil }) if err != nil { return nil, err } return p.store.GetTransactionWithVolumes(ctx, GetTransactionQuery{ ... }) }📝 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.func (p transactionsPaginatedResourceAdapter) GetOne(ctx context.Context, query ledgercontroller.ResourceQuery[any]) (*ledger.Transaction, error) { var txID int err := query.Builder.Walk(func(_ string, _ string, value any) error { i, ok := value.(int) if !ok { return fmt.Errorf("expected int but got %T", value) } txID = i return nil }) if err != nil { return nil, err } return p.store.GetTransactionWithVolumes(ctx, GetTransactionQuery{ PITFilterWithVolumes: PITFilterWithVolumes{ PITFilter: PITFilter{ PIT: query.PIT, OOT: query.OOT, }, ExpandVolumes: slices.Contains(query.Expand, "volumes"), ExpandEffectiveVolumes: slices.Contains(query.Expand, "effectiveVolumes"), }, ID: txID, }) }
gfyrag
force-pushed
the
hotfix/import-from-2.1
branch
3 times, most recently
from
283e5eb to
20edf69
Compare
gfyrag
force-pushed
the
hotfix/import-from-2.1
branch
from
20edf69 to
e9ba16a
Compare
No description provided.