chore: add openapi tag #676
Conversation
WalkthroughThe changes update the API’s routing and endpoint configuration across multiple packages. In the v2 router, the NewRouter function signature now includes an additional parameter (often "develop") and several new options to support bulk processing and tracing. The visibility of the v1 GetInfo function has been modified, and its usage has shifted in routing. SQL migration scripts have been enhanced with new functions, triggers, and updated encoding. Client SDK documentation and methods have been revised to utilize versioned endpoints, and new paginated resource adapters have been added to the ledger storage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant V2Router
participant V1API
Client->>V2Router: GET /_info
V2Router->>V1API: Call GetInfo(systemController, version)
V1API-->>V2Router: Return Info Response
V2Router-->>Client: HTTP 200 with Info Response
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🔇 Additional comments (1)
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.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
internal/storage/ledger/logs.go (1)
69-77:🧩 Analysis chain
Verify error handling in memento serialization.
The new memento handling logic needs to handle potential type assertion failures and serialization errors gracefully.
Consider this improved error handling:
mementoObject := log.Data.(any) if memento, ok := mementoObject.(ledger.Memento); ok { mementoObject = memento.GetMemento() } mementoData, err := json.Marshal(mementoObject) if err != nil { - return err + return fmt.Errorf("failed to marshal memento data: %w", err) }
🏁 Script executed:
#!/bin/bash # Output the relevant lines (60-85) from the logs.go file to verify the current error handling. sed -n '60,85p' internal/storage/ledger/logs.goLength of output: 622
Action: Enhance error handling for memento serialization
The current implementation in internal/storage/ledger/logs.go wraps errors for marshalling log data but not for marshalling memento data. Consistency in error context is important, so the memento serialization error should be wrapped with additional context.
In lines 74–77, update the error handling from:
─────────────────────────────
mementoData, err := json.Marshal(mementoObject) if err != nil { return err }─────────────────────────────
To the improved pattern:
─────────────────────────────
mementoData, err := json.Marshal(mementoObject) if err != nil { return fmt.Errorf("failed to marshal memento data: %w", err) }─────────────────────────────
This change will provide better context in error messages, aiding debugging.
test/e2e/app_lifecycle_test.go (1)
144-144: Update GetInfo call to use V2 client.This line still uses the old
GetInfofunction while the rest of the codebase has moved to using the V2 client.Update the code to use the V2 client:
- _, err := GetInfo(ctx, testServer.GetValue()) + _, err := testServer.GetValue().Client().Ledger.V2.GetInfo(ctx)pkg/client/v2.go (1)
402-405: Fix duplicate OAuth2 scopes in all methods.Multiple methods have duplicate OAuth2 scopes. This should be fixed throughout the file.
Remove duplicate
"ledger:read"entries from all OAuth2Scopes arrays in the following methods:
- ListLedgers
- GetLedger
- CreateLedger
- GetLedgerInfo
- CountAccounts
- ListAccounts
- GetAccount
- AddMetadataToAccount
- DeleteAccountMetadata
- ReadStats
- CountTransactions
- ListTransactions
- CreateTransaction
- GetTransaction
- AddMetadataOnTransaction
- DeleteTransactionMetadata
- RevertTransaction
- GetBalancesAggregated
- GetVolumesWithBalances
- ListLogs
- ImportLogs
- ExportLogs
Also applies to: 587-588, 766-767, 1294-1295, 1664-1665, 1845-1846, 2033-2034, 2215-2216, 2397-2398, 2565-2568, 2744-2747, 2925-2928, 3114-3117, 3305-3308, 3487-3490, 3669-3672, 3837-3840, 4020-4023, 4209-4212, 4399-4402, 4586-4589, 4761-4764
internal/storage/bucket/migrations/23-logs-fill-memento/up.sql (1)
473-529: Dynamic Creation of Sequences and Triggers (DO Block)
Within a procedural block, the migration iterates over records from “_system.ledgers” and dynamically creates sequences (for transactions and logs) and triggers for each ledger.
• This dynamic SQL is clever; however, be sure to validate that ledger names and IDs are sanitized to avoid SQL‑injection risks. Even if the source is internal, a parameterized approach or proper quoting may improve safety.
🧹 Nitpick comments (20)
test/e2e/api_transactions_create_test.go (1)
85-88: Add explicit metadata validation in test assertions.While adding special characters in metadata is good for testing edge cases, the test assertions don't explicitly verify that these values are preserved correctly.
Consider adding specific assertions for metadata values in the test at line 115:
Expect(*response).To(Equal(components.V2Transaction{ Timestamp: rsp.Timestamp, InsertedAt: rsp.InsertedAt, Postings: rsp.Postings, Reference: rsp.Reference, - Metadata: rsp.Metadata, + Metadata: map[string]string{ + "add some quotes": "\" quoted value\"", + "add some utf-8 characters": "½", + }, ID: rsp.ID,internal/storage/ledger/legacy/adapters.go (6)
76-78: Avoid panic in “never used” methods.
Panicking can be risky in production. Returning an error might be safer in case this function is unexpectedly called.-func (p logsPaginatedResourceAdapter) GetOne(_ context.Context, _ ledgercontroller.ResourceQuery[any]) (*ledger.Log, error) { - panic("never used") -} +func (p logsPaginatedResourceAdapter) GetOne(_ context.Context, _ ledgercontroller.ResourceQuery[any]) (*ledger.Log, error) { + return nil, errors.New("GetOne is not implemented") +}
80-82: Consider returning an error instead of panic.
Similar to GetOne, returning an error in Count method avoids unintended panics if the method is called.
157-181: Simplify big.Int addition.
new(big.Int).Add(new(big.Int), balance) is effectively just copying “balance.” Consider using Set(balance) or retrieving directly if suitable.-Input: new(big.Int).Add(new(big.Int), balance), +Input: new(big.Int).Set(balance),
183-185: Avoid panic in Count.
If this is truly never called, returning an error provides safer fallback than panicking.
208-210: Replace panic with error return.
Maintaining a consistent pattern of returning errors here might be safer than panicking.
212-214: Consider returning error in Count.
Continuing the approach above, returning an error is gentler than a panic.internal/api/v2/controllers_ledgers_read_test.go (1)
25-25: Consider using a constant for the version string.The hardcoded "develop" string could make tests less maintainable. Consider defining a constant for this version string to make it easier to update across all tests.
+const testVersion = "develop" + func TestLedgersRead(t *testing.T) { // ... - router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") + router := NewRouter(systemController, auth.NewNoAuth(), testVersion, os.Getenv("DEBUG") == "true")test/e2e/app_multiple_instance_test.go (1)
53-55: Ensure version string consistency.The version check should use the same constant as suggested for the router initialization to maintain consistency across tests.
+const testVersion = "develop" + It("each service should be up and running", func() { for server := range servers { info, err := server.Client().Ledger.V2.GetInfo(ctx) Expect(err).NotTo(HaveOccurred()) - Expect(info.V2ConfigInfoResponse.Version).To(Equal("develop")) + Expect(info.V2ConfigInfoResponse.Version).To(Equal(testVersion)) } })internal/api/v1/controllers_config.go (1)
35-65: LGTM! Consider adding documentation for the exported function.The function logic is correct and well-structured. Since this is now an exported function, consider adding a documentation comment to describe its purpose and usage.
Add a documentation comment above the function:
+// GetInfo returns an HTTP handler that provides system configuration information including +// the server version and available ledgers. It uses the provided systemController to +// retrieve ledger information and returns it in a structured format. func GetInfo(systemController system.Controller, version string) func(w http.ResponseWriter, r *http.Request) {internal/api/v2/controllers_transactions_delete_metadata_test.go (1)
71-71: Consider using a constant for the environment parameter.The hardcoded "develop" string could be replaced with a constant to improve maintainability and prevent typos across test files.
+const testEnvironment = "develop" + -router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") +router := NewRouter(systemController, auth.NewNoAuth(), testEnvironment, os.Getenv("DEBUG") == "true")internal/storage/ledger/logs.go (1)
25-25: Consider documenting the purpose of the Memento field.The change from
RawMessageto[]bytefor theMementofield type is a significant change that warrants documentation explaining its purpose and usage.Add a comment above the field:
+// Memento stores the serialized state of the log entry Memento []byte `bun:"memento,type:bytea"`internal/api/v2/controllers_accounts_count_test.go (1)
141-141: Consider using a constant for API version.The hardcoded "develop" string could be replaced with a constant or configuration value for better maintainability and flexibility.
Consider defining a constant:
- router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") +const ( + APIVersion = "develop" +) + router := NewRouter(systemController, auth.NewNoAuth(), APIVersion, os.Getenv("DEBUG") == "true")internal/api/v2/routes.go (2)
22-28: Document the version parameter.The new version parameter is added to support API versioning, but its purpose and expected values should be documented.
Add documentation for the version parameter:
func NewRouter( systemController system.Controller, authenticator auth.Authenticator, - version string, + // version specifies the API version (e.g., "develop", "v1", "v2") + version string, debug bool, opts ...RouterOption, ) chi.Router {
41-42: Document v1.GetInfo reuse.The reuse of v1.GetInfo in the v2 router should be documented to explain the intentional version compatibility.
Add a comment explaining the version compatibility:
+ // Reuse v1 info endpoint for version compatibility router.Get("/_info", v1.GetInfo(systemController, version))internal/api/v2/controllers_bulk_test.go (1)
488-488: Consider making the version parameter configurable.The hardcoded "develop" version might limit test flexibility. Consider:
- Using a constant or environment variable to make it configurable.
- Adding test cases to verify router behavior with different version values.
Apply this diff to make the version configurable:
- router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") + version := os.Getenv("API_VERSION") + if version == "" { + version = "develop" + } + router := NewRouter(systemController, auth.NewNoAuth(), version, os.Getenv("DEBUG") == "true")pkg/client/v2.go (1)
1-1: Consider adding API versioning information.Since this is a generated file, consider adding API version information in the header comment to help track compatibility.
Apply this diff to add version info:
-// Code generated by Speakeasy (https://speakeasy.com). DO NOT EDIT. +// Code generated by Speakeasy (https://speakeasy.com). DO NOT EDIT. +// API Version: v2pkg/client/README.md (1)
72-76: Updated SDK Example Usage in README
The example usage now calls “s.Ledger.V1.GetInfo(ctx)” with an updated response check “if res.ConfigInfoResponse != nil”. This correction aligns with the API’s new versioned interface and improved error handling.
• Also, consider checking for any hard‑tab formatting flagged by markdownlint and replace them with spaces where appropriate.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
72-72: Hard tabs
Column: 1(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1(MD010, no-hard-tabs)
pkg/client/docs/sdks/v2/README.md (2)
6-8: Updated Operation List with New V2 Methods
The operations list now includes “GetInfo” and “GetMetrics” for the V2 Ledger API.
• Ensure that the bullet style (dashes are preferred per MD004) and surrounding blank lines meet markdownlint standards.
85-119: GetMetrics Documentation – Example Usage and Response Structure
The “GetMetrics” section appears clear, with an example demonstrating retrieving in‑memory metrics.
• Confirm that the response check “if res.Object != nil” correctly matches the returned response structure.
• Also, consider adding surrounding blank lines before/after tables per markdownlint (MD058) guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
95-95: Hard tabs
Column: 1(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1(MD010, no-hard-tabs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/main.ymlis excluded by!**/*.ymlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/gen.yamlis excluded by!**/*.yaml
📒 Files selected for processing (48)
internal/api/router.go(1 hunks)internal/api/v1/controllers_config.go(1 hunks)internal/api/v1/routes.go(1 hunks)internal/api/v2/controllers_accounts_add_metadata_test.go(1 hunks)internal/api/v2/controllers_accounts_count_test.go(1 hunks)internal/api/v2/controllers_accounts_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_accounts_list_test.go(1 hunks)internal/api/v2/controllers_accounts_read_test.go(1 hunks)internal/api/v2/controllers_balances_test.go(1 hunks)internal/api/v2/controllers_bulk_test.go(1 hunks)internal/api/v2/controllers_ledgers_create_test.go(1 hunks)internal/api/v2/controllers_ledgers_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_ledgers_info_test.go(1 hunks)internal/api/v2/controllers_ledgers_list_test.go(1 hunks)internal/api/v2/controllers_ledgers_read_test.go(1 hunks)internal/api/v2/controllers_ledgers_update_metadata_test.go(1 hunks)internal/api/v2/controllers_logs_export_test.go(1 hunks)internal/api/v2/controllers_logs_import_test.go(1 hunks)internal/api/v2/controllers_logs_list_test.go(1 hunks)internal/api/v2/controllers_stats_test.go(1 hunks)internal/api/v2/controllers_transactions_add_metadata_test.go(1 hunks)internal/api/v2/controllers_transactions_count_test.go(1 hunks)internal/api/v2/controllers_transactions_create_test.go(1 hunks)internal/api/v2/controllers_transactions_delete_metadata_test.go(1 hunks)internal/api/v2/controllers_transactions_list_test.go(1 hunks)internal/api/v2/controllers_transactions_read_test.go(1 hunks)internal/api/v2/controllers_transactions_revert_test.go(1 hunks)internal/api/v2/controllers_volumes_test.go(1 hunks)internal/api/v2/routes.go(3 hunks)internal/controller/ledger/log_process.go(1 hunks)internal/storage/bucket/migrations/11-make-stateless/up.sql(1 hunks)internal/storage/bucket/migrations/23-logs-fill-memento/up.sql(1 hunks)internal/storage/ledger/legacy/adapters.go(1 hunks)internal/storage/ledger/logs.go(1 hunks)internal/storage/ledger/logs_test.go(2 hunks)internal/storage/ledger/transactions_test.go(1 hunks)pkg/client/README.md(10 hunks)pkg/client/USAGE.md(1 hunks)pkg/client/docs/sdks/ledger/README.md(0 hunks)pkg/client/docs/sdks/v2/README.md(2 hunks)pkg/client/formance.go(1 hunks)pkg/client/ledger.go(0 hunks)pkg/client/v2.go(1 hunks)pkg/testserver/api.go(1 hunks)test/e2e/api_transactions_create_test.go(2 hunks)test/e2e/app_lifecycle_test.go(1 hunks)test/e2e/app_multiple_instance_test.go(1 hunks)test/performance/benchmark_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/client/docs/sdks/ledger/README.md
- pkg/client/ledger.go
✅ Files skipped from review due to trivial changes (3)
- internal/storage/ledger/transactions_test.go
- internal/api/v2/controllers_transactions_create_test.go
- pkg/client/formance.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
pkg/client/USAGE.md
21-21: Hard tabs
Column: 1
(MD010, no-hard-tabs)
22-22: Hard tabs
Column: 1
(MD010, no-hard-tabs)
23-23: Hard tabs
Column: 1
(MD010, no-hard-tabs)
24-24: Hard tabs
Column: 1
(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1
(MD010, no-hard-tabs)
pkg/client/README.md
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
74-74: Hard tabs
Column: 1
(MD010, no-hard-tabs)
75-75: Hard tabs
Column: 1
(MD010, no-hard-tabs)
76-76: Hard tabs
Column: 1
(MD010, no-hard-tabs)
112-112: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
113-113: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
168-168: Hard tabs
Column: 1
(MD010, no-hard-tabs)
182-182: Hard tabs
Column: 1
(MD010, no-hard-tabs)
221-221: Hard tabs
Column: 1
(MD010, no-hard-tabs)
222-222: Hard tabs
Column: 1
(MD010, no-hard-tabs)
223-223: Hard tabs
Column: 1
(MD010, no-hard-tabs)
224-224: Hard tabs
Column: 1
(MD010, no-hard-tabs)
225-225: Hard tabs
Column: 1
(MD010, no-hard-tabs)
266-266: Hard tabs
Column: 1
(MD010, no-hard-tabs)
267-267: Hard tabs
Column: 1
(MD010, no-hard-tabs)
269-269: Hard tabs
Column: 1
(MD010, no-hard-tabs)
319-319: Hard tabs
Column: 1
(MD010, no-hard-tabs)
320-320: Hard tabs
Column: 1
(MD010, no-hard-tabs)
321-321: Hard tabs
Column: 1
(MD010, no-hard-tabs)
322-322: Hard tabs
Column: 1
(MD010, no-hard-tabs)
323-323: Hard tabs
Column: 1
(MD010, no-hard-tabs)
354-354: Hard tabs
Column: 1
(MD010, no-hard-tabs)
355-355: Hard tabs
Column: 1
(MD010, no-hard-tabs)
356-356: Hard tabs
Column: 1
(MD010, no-hard-tabs)
357-357: Hard tabs
Column: 1
(MD010, no-hard-tabs)
358-358: Hard tabs
Column: 1
(MD010, no-hard-tabs)
433-433: Hard tabs
Column: 1
(MD010, no-hard-tabs)
434-434: Hard tabs
Column: 1
(MD010, no-hard-tabs)
435-435: Hard tabs
Column: 1
(MD010, no-hard-tabs)
436-436: Hard tabs
Column: 1
(MD010, no-hard-tabs)
437-437: Hard tabs
Column: 1
(MD010, no-hard-tabs)
pkg/client/docs/sdks/v2/README.md
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1
(MD010, no-hard-tabs)
80-80: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
95-95: Hard tabs
Column: 1
(MD010, no-hard-tabs)
96-96: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Tables should be surrounded by blank lines
null
(MD058, blanks-around-tables)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (87)
test/e2e/api_transactions_create_test.go (1)
25-25: LGTM! Context name accurately reflects test purpose.The updated context name "Ledger transactions create API tests" better describes the actual test scenarios in this file.
internal/storage/ledger/legacy/adapters.go (25)
6-6: Use of bunpaginate import looks good.
No immediate issues identified with the import statement here.
14-15: Confirm Go compatibility for “math/big” and “slices” imports.
“math/big” is standard in all supported Go versions. However, “slices” was introduced in Go 1.21. Ensure the project’s Go version is aligned or that a polyfill is used if earlier versions are required.
18-20: Struct definition approved.
Defining a dedicated struct for account pagination adapter is clear and aligns well with your design.
41-53: Counting accounts is straightforward.
No issues spotted with the code logic here.
55-68: Paginate method logic is consistent.
Implementation follows a clear pattern for retrieving paginated results.
70-70: Interface assertion recognized.
Declaring the adapter to satisfy the interface clarifies usage.
72-74: Struct definition approved.
Implementation for logs adapter is consistent with the pattern in other adapters.
84-95: Logs Paginate looks good.
The store-based pagination logic is consistent.
97-97: Interface assertion recognized.
Implementation satisfies ledgercontroller.PaginatedResource for logs.
99-101: Struct definition approved.
transactionsPaginatedResourceAdapter follows the established pattern.
103-120: Check for potential panic with int casting.
As with accounts, ensure that the type assertion to int is safe. Consider adding type checks to avoid runtime panic if an unexpected type is passed.
122-134: Counting transactions is clean.
Logic is straightforward and consistent with the store approach.
136-149: Pagination logic for transactions is fine.
Implementation matches other adapters.
151-151: Interface confirmation approved.
Adapter meets PaginatedResource interface.
153-155: Struct definition approved.
aggregatedBalancesPaginatedResourceAdapter provides a consistent approach to storing references.
187-187: Interface assertion recognized.
Adapter is declared to satisfy ledgercontroller.Resource.
189-191: Struct definition approved.
aggregatedVolumesPaginatedResourceAdapter continues the standard approach.
193-206: Volumes pagination logic is correct.
Implementation properly delegates to the store function for retrieving volumes with balances.
216-216: Interface compliance reaffirmed.
No issues with the interface implementation.
225-227: Conditional legacy store usage for Accounts is clear.
Switching to the new store if fully up to date is a good approach.
232-234: Conditional usage of logsPaginatedResourceAdapter.
Matches the pattern used for Accounts.
239-241: Conditional usage of transactionsPaginatedResourceAdapter.
Consistent with the design pattern for choosing between legacy/new store.
245-247: AggregatedBalances fallback is consistent.
Good approach for handling legacy vs. new store.
253-255: Volumes fallback is consistent.
Continues the pattern for legacy store usage.
22-39: 🛠️ Refactor suggestionValidate type-casting in Builder.Walk.
Currently, the code performs 'value.(string)' without error handling, which may panic if the input is invalid. Consider adding a type-check or fallback error handling to guard against unexpected types.-_ = query.Builder.Walk(func(_ string, _ string, value any) error { - address = value.(string) - return nil -}) +err := query.Builder.Walk(func(_ string, _ string, value any) error { + strVal, ok := value.(string) + if !ok { + return fmt.Errorf("expected string, got %T", value) + } + address = strVal + return nil +}) +if err != nil { + return nil, err +}internal/api/v2/controllers_ledgers_update_metadata_test.go (1)
29-29: New router parameter: “develop”.
Including the “develop” version string ensures correct instantiation of the router for testing. Check that the additional parameter is properly handled in NewRouter to avoid misconfiguration.internal/api/v2/controllers_stats_test.go (1)
20-20: Added “develop” parameter in NewRouter.
Passing the string “develop” aligns with the new router signature requiring a version. Confirm that this correctly reflects the intended environment.internal/api/v2/controllers_transactions_read_test.go (1)
40-40: Use constant for version string.Same feedback as in
controllers_ledgers_read_test.goabout using a constant for the version string.internal/api/v2/controllers_ledgers_info_test.go (1)
22-22: Use constant for version string.Same feedback as in
controllers_ledgers_read_test.goabout using a constant for the version string.internal/api/v2/controllers_ledgers_delete_metadata_test.go (1)
57-57: LGTM! Router initialization updated correctly.The addition of the "develop" parameter to NewRouter is consistent with the API versioning changes.
internal/api/v2/controllers_logs_export_test.go (1)
65-65: LGTM! Router initialization updated correctly.The addition of the "develop" parameter to NewRouter is consistent with the API versioning changes.
internal/api/v2/controllers_logs_import_test.go (1)
76-76: LGTM! Router initialization updated correctly.The addition of the "develop" parameter to NewRouter is consistent with the API versioning changes.
internal/api/v2/controllers_accounts_delete_metadata_test.go (1)
75-75: LGTM!The change is consistent with other test files, maintaining uniformity across the codebase.
internal/api/v2/controllers_transactions_revert_test.go (1)
84-84: LGTM!The change aligns with the broader effort to add OpenAPI tags and versioning support.
internal/api/v1/routes.go (1)
32-32: Verify the implications of makingGetInfopublic.The function has been made public (exported) to be used by other packages. Ensure that this change is intentional and that the function's contract is well-defined for external use.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if GetInfo is used by other packages # Search for usage of GetInfo function ast-grep --pattern 'GetInfo($_, $_)'Length of output: 4060
Public Interface Usage Confirmed
The output confirms that the newly exported GetInfo function is being used externally—in particular, it’s invoked in the test at test/e2e/app_lifecycle_test.go (line 144) as well as in the routes configuration. This external usage indicates that making GetInfo public is an intentional change. Please ensure that the function’s contract (expected parameters, behavior, error handling, etc.) is clearly documented for any external consumers.
internal/api/v2/controllers_transactions_add_metadata_test.go (1)
102-102: LGTM!The update to include the "develop" parameter aligns with the router signature changes.
internal/api/v2/controllers_accounts_read_test.go (1)
96-96: LGTM!The update to include the "develop" parameter aligns with the router signature changes.
internal/api/v2/controllers_balances_test.go (1)
103-103: LGTM!The update to include the "develop" parameter aligns with the router signature changes.
internal/api/v2/controllers_ledgers_list_test.go (1)
98-98: LGTM!The addition of the version parameter "develop" aligns with the PR objective.
internal/api/router.go (1)
67-67: LGTM!The version parameter is properly propagated to both v1 and v2 routers.
internal/api/v2/controllers_ledgers_create_test.go (1)
103-103: LGTM!The addition of the version parameter "develop" is consistent with other test files.
test/performance/benchmark_test.go (1)
120-120: Verify API version change impact.The client call has been updated to use the V2 API endpoint. This is a breaking change that requires clients to update their code.
Run the following script to check for any remaining unversioned API calls:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining unversioned GetMetrics calls rg -A 5 'Client\(\)\.Ledger\.GetMetrics'Length of output: 42
I'll run an additional script to capture all usages (both versioned and unversioned) of GetMetrics for a more robust check:
🏁 Script executed:
#!/bin/bash # Description: Search for both versioned and unversioned GetMetrics calls to verify no unversioned calls remain. rg -A 5 'Client\(\)\.Ledger\.(V2\.)?GetMetrics'Length of output: 579
API Version Change Impact Verified
The search confirmed that the only call present is to the V2 API endpoint. No unversioned calls using
env.Client().Ledger.GetMetrics(ctx)were found.internal/controller/ledger/log_process.go (1)
140-140: LGTM! Improved string emptiness check.The change to use
len()is more idiomatic in Go for checking string emptiness.internal/api/v2/controllers_transactions_count_test.go (1)
167-167: LGTM!The change to add the "develop" parameter to the
NewRouterfunction call aligns with the API's routing configuration updates.internal/api/v2/controllers_volumes_test.go (1)
153-153: LGTM!The change to add the "develop" parameter to the
NewRouterfunction call aligns with the API's routing configuration updates.internal/api/v2/controllers_logs_list_test.go (1)
177-177: LGTM!The change to add the "develop" parameter to the
NewRouterfunction call aligns with the API's routing configuration updates.internal/storage/ledger/logs_test.go (2)
26-26: LGTM! Function renames improve consistency.The renaming of test functions to include "Logs" in their names (
TestInsertLog->TestLogsInsertandTestReadLogWithIdempotencyKey->TestLogsReadWithIdempotencyKey) improves naming consistency across the test suite.Also applies to: 171-171
142-168: LGTM! Well-structured test case for special characters.The new test case for special characters in metadata:
- Uses table-driven test pattern for better maintainability
- Covers important edge cases: escaped quotes and UTF-8 characters
- Runs tests in parallel for better performance
internal/api/v2/controllers_accounts_list_test.go (1)
221-221: LGTM! Verify version parameter usage.The router initialization has been updated to include the version parameter. The change is consistent with the updated
NewRouterfunction signature.Let's verify how the version parameter is used in the router:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check how the version parameter is used in the router. rg -A 5 'NewRouter.*version.*' internal/api/v2/Length of output: 49
I'm not seeing any output with the initial search. I'll run a new script to locate the definition of NewRouter within internal/api/v2 and review how the version parameter is used. Please hold on.
🏁 Script executed:
#!/bin/bash # Locate the NewRouter function definition with some context to see how the version parameter is handled. rg -C 5 "func NewRouter(" internal/api/v2/Length of output: 125
Let's re-run the search using a literal match for the NewRouter definition. This will help us locate its implementation and examine how the version parameter is handled.
🏁 Script executed:
#!/bin/bash # Search for the NewRouter function definition across the repository using a literal match. rg -F "func NewRouter(" -n .Length of output: 168
🏁 Script executed:
#!/bin/bash # Show context for the NewRouter function in internal/api/v2/routes.go to inspect the version parameter usage. rg -C 10 "func NewRouter(" internal/api/v2/routes.goLength of output: 135
🏁 Script executed:
#!/bin/bash # Re-run the search for the NewRouter function definition in internal/api/v2/routes.go using fixed string matching. rg -F -C 10 "func NewRouter(" internal/api/v2/routes.goLength of output: 575
Router Initialization Version Parameter Verified
The extracted function definition in internal/api/v2/routes.go confirms that the NewRouter function signature now accepts the third parameter as a string (version). The test file's router initialization call:
router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true")
correctly uses the updated signature by passing the "develop" version parameter.
pkg/testserver/api.go (1)
30-30: LGTM! Using versioned API endpoint.The function now uses the V2 client for getting info, which is consistent with the standardization of API versioning.
internal/api/v2/controllers_transactions_list_test.go (1)
262-262: LGTM! Router initialization updated.The router initialization has been updated to include the version parameter, consistent with the changes across other test files.
test/e2e/app_lifecycle_test.go (1)
52-52: LGTM! Using versioned API endpoint.The function now uses the V2 client for getting info, consistent with the API versioning standardization.
pkg/client/v2.go (2)
30-219: Verify the GetInfo implementation.The GetInfo method implementation looks correct but has some potential improvements:
- Consider adding request validation before making the HTTP call
- Consider adding response caching for frequently accessed server info
- Consider adding metrics/tracing for monitoring API performance
221-398: Verify the GetMetrics implementation.The GetMetrics method implementation looks correct but has some potential improvements:
- Consider adding request validation before making the HTTP call
- Consider adding response caching with configurable TTL for metrics data
- Consider adding metrics/tracing for monitoring API performance
pkg/client/USAGE.md (1)
21-25: Updated V1 API call and response check in the SDK example usage
The example now calls “s.Ledger.V1.GetInfo(ctx)” and checks for a non‑nil “res.ConfigInfoResponse”. This matches the new versioned API design. Please double‑check that all related examples and documentation (especially in other client files) are now fully consistent with the versioning changes.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
21-21: Hard tabs
Column: 1(MD010, no-hard-tabs)
22-22: Hard tabs
Column: 1(MD010, no-hard-tabs)
23-23: Hard tabs
Column: 1(MD010, no-hard-tabs)
24-24: Hard tabs
Column: 1(MD010, no-hard-tabs)
25-25: Hard tabs
Column: 1(MD010, no-hard-tabs)
internal/storage/bucket/migrations/23-logs-fill-memento/up.sql (29)
1-24: Transaction Date Function – Consistent Timestamp Calculation
The new function “transaction_date()” creates (if needed) a temporary table, retrieves a timestamp (or stores one if none exists), and then returns it in UTC. This design provides a consistent statement timestamp within the transaction.
• Verify that the creation of a temporary table each time this function is called does not introduce performance issues and that its behavior is as intended in concurrent sessions.
26-31: Dropping legacy triggers
Triggers on “accounts”, “transactions”, and “logs” are dropped. This cleanup appears appropriate given the schema changes.
32-39: Altering the “moves” Table – Adding Column and Adjusting Constraints
The “moves” table is altered to add a “transactions_id” (bigint) column and to drop NOT NULL constraints on several volume columns, while also setting default values using “transaction_date()”.
• Please verify that these schema changes preserve data integrity and that any application logic reading from “moves” is updated accordingly.
40-45: Renaming Columns in “moves” for Consistency
Columns “account_address” and “account_address_array” are being renamed to “accounts_address” and “accounts_address_array”.
• This renaming improves naming consistency – ensure that all queries and functions referencing these columns have been updated.
46-62: get_aggregated_volumes_for_transaction Function
The new SQL function aggregates volumes by building a JSON object from distinct “accounts_address” and “asset” fields.
• Confirm that the use of “distinct on” and the call to “volumes_to_jsonb(...)" return the expected results under all scenarios.
63-78: get_aggregated_effective_volumes_for_transaction Function
Similarly, this function builds a JSON object for effective volumes. The structure is analogous to the previous function.
• Ensure that “first(move.post_commit_effective_volumes)” correctly represents the effective volumes for aggregation.
80-102: get_all_account_effective_volumes Function
This function utilises WITH clauses (including lateral joins) to fetch the most recent move (by effective_date and sequence) for every asset.
• Verify that performance remains acceptable on large datasets and that the “_before” conditional logic meets business needs.
104-126: get_all_account_volumes Function
A similar construction to the previous function but uses “insertion_date”.
• Confirm that ordering by “seq desc” produces the correct result for the volume’s current state.
128-150: set_compat_on_move Function and Trigger
This function sets “transactions_seq” and “accounts_seq” (and derives a JSON “accounts_address_array”) before inserting into “moves”.
• Review the subqueries for performance and correctness, ensuring that the lookup on “transactions” and “accounts” reliably retrieves the correct sequence numbers.
152-172: set_compat_on_accounts_metadata Function and Trigger
This function is used to set “accounts_seq” on the “accounts_metadata” table.
• Ensure that the join condition “where address = new.accounts_address” is correct given the recent renaming of address fields.
174-194: set_compat_on_transactions_metadata Function and Trigger
Analogous to the previous function, this one ensures “transactions_seq” is set on “transactions_metadata”.
• Double-check that the conditions match the updated ledger schema.
201-207: Altering the “transactions” Table
New columns “post_commit_volumes” (jsonb) and “inserted_at” (timestamp) are added and the “timestamp” column’s default is set to transaction_date().
• Be cautious that changing a column’s default on a live table may lock it; ensure that any migration downtime is acceptable.
209-214: Altering the “logs” Table
Columns “memento” (bytea) and “idempotency_hash” (bytea) are added, while the “hash” column loses its NOT NULL constraint and “date” gets a default.
• This update—changing the encoding logic—is critical; please test that downstream consumers now correctly interpret “memento” data encoded as UTF-8.
215-220: Altering the “accounts” Table
Defaults are updated for “first_usage”, “insertion_date”, and “updated_at”, and the NOT NULL constraint is dropped from “address_array”.
• Ensure that these changes align with the intended behavior (e.g. when the account is first used) and that the application code handles NULL values appropriately.
222-230: Creation of “accounts_volumes” Table
A new table to store input/output volume data per ledger, account, and asset is created with a composite primary key.
• Verify that the primary key choice supports the workload and that there is an index (automatically via PK) to help with join conditions in related queries.
232-237: Altering Metadata Tables
Additional columns (“transactions_id” in transactions_metadata, “accounts_address” in accounts_metadata) are added.
• Confirm that these columns are populated by the corresponding triggers and that backward compatibility is maintained.
238-264: set_effective_volumes Function
This trigger function calculates new “post_commit_effective_volumes” by coalescing a prior value (or a new one based on “new.amount” and “new.is_source”).
• Carefully verify the arithmetic logic here. In particular, ensure that when no previous record exists the default amounts are correctly used.
266-285: update_effective_volumes Function
This function updates descendant “moves” records (with later effective dates) by incrementing their volumes.
• Validate that the WHERE clause conditions (“effective_date > new.effective_date”) correctly identify the records that need updating.
287-304: update_transaction_metadata_history Function
On update, this function inserts a new revision into “transactions_metadata” by incrementing the previous revision.
• Consider potential race conditions if multiple updates occur concurrently for the same transaction. A locking strategy may be needed if conflicts are detected.
306-317: insert_transaction_metadata_history Function
This function handles the initial insertion (revision 1) into “transactions_metadata”.
• The logic is straightforward; verify that it’s triggered only for new transactions.
319-336: update_account_metadata_history Function
Similarly, this function updates “accounts_metadata” by inserting a new revision.
• As with transactions, review for potential concurrency issues.
338-349: insert_account_metadata_history Function
This function inserts the initial metadata record for an account with revision 1.
• Ensure that the insertion_date and metadata fields match the required format.
351-362: explode_address Function
Returns a JSONB object constructed by splitting an address string on “:” and aggregating the pieces.
• The use of “public.aggregate_objects” is creative; just verify that the ordering (row_number()-1) meets any client expectations.
364-374: set_transaction_inserted_at Function
A simple trigger function that sets “inserted_at” using the “transaction_date()” function.
376-400: set_transaction_addresses Function
This function populates “sources” and “destinations” from a JSON postings array.
• Confirm that the use of “jsonb_array_elements” and the aggregation produces an array in the intended format.
402-418: set_transaction_addresses_segments Function
Updates “sources_arrays” and “destinations_arrays” by applying the custom “explode_address” function for each posting.
• This segmentation can be hard to test—please verify its accuracy on a representative sample of posting data.
421-431: set_address_array_for_account Function
Sets the “address_array” field by converting the address string into a JSON array.
433-471: set_log_hash Function – Computing a Log Hash with UTF-8 Encoding
This function calculates a new hash for a log record. It retrieves the previous hash and then builds a JSON string (via string concatenation) from selected fields. In the CASE expression, if there is no previous hash, it converts “marshalledAsJSON” to bytea; otherwise, it prepends the previous hash (encoded in base64) and then appends the current JSON (converted with UTF-8).
• The hashing logic is non‑trivial. Consider adding inline comments to explain the reasoning behind the concatenation and base64 conversion for maintainability.
531-560: Enforcing Reference Uniqueness with a Trigger
The function “enforce_reference_uniqueness()” and its associated constraint trigger ensure that no duplicate transaction references exist within the same ledger.
• The use of “pg_advisory_xact_lock” is a neat trick to serialize this check, but please verify that it does not introduce performance bottlenecks under heavy insert loads.internal/storage/bucket/migrations/11-make-stateless/up.sql (1)
1-560: Overall SQL Migration Review
This migration file makes extensive schema and function changes to support a stateless design and better data consistency.
• The functions (from “transaction_date()” to the metadata trigger functions) as well as the ALTER TABLE statements appear logically grouped and clearly update the previous encoding (from LATIN1 to UTF-8), default timestamps, and column constraints.
• Please ensure any inter‑dependency (for example, the triggers that populate metadata history) works in tandem under load.
• It is recommended to thoroughly test these changes in a staging environment with realistic data volumes to verify that performance and transactional integrity hold.pkg/client/docs/sdks/v2/README.md (1)
34-68:⚠️ Potential issueGetInfo Documentation – Example Usage and Details
The “GetInfo” section now provides an example using “s.Ledger.V2.GetInfo(ctx)”. Note that the current example checks “if res.V2ConfigInfoResponse != nil”, but the response type documented is “*operations.V2GetInfoResponse”.
• Please update the field name in the example (e.g. to “res.V2GetInfoResponse”) so that it is consistent with the response object definition.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
44-44: Hard tabs
Column: 1(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1(MD010, no-hard-tabs)
| } | ||
|
|
||
| router := NewRouter(systemController, auth.NewNoAuth(), os.Getenv("DEBUG") == "true") | ||
| router := NewRouter(systemController, auth.NewNoAuth(), "develop", os.Getenv("DEBUG") == "true") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the purpose of the environment parameter.
The addition of the "develop" parameter suggests environment-specific behavior. Please ensure this is documented in the NewRouter function's documentation.
🏁 Script executed:
#!/bin/bash
# Search for NewRouter documentation
ast-grep --pattern 'func NewRouter($_, $_, $_, $_) {
$$$
}'Length of output: 63
I'll run an additional script to locate the definition of the NewRouter function along with its surrounding documentation. This should help verify if the environment parameter ("develop") is documented in the NewRouter documentation.
🏁 Script executed:
#!/bin/bash
# Search for the definition of NewRouter along with some context to inspect its documentation.
rg -n -C 5 "func NewRouter" .Length of output: 1811
Action Required: Document the "version" Parameter in NewRouter
Investigation shows that the NewRouter function is declared in multiple files (e.g., internal/api/router.go, internal/api/v2/routes.go, and internal/api/v1/routes.go) with a fourth parameter of type bool for debug and a third string parameter (currently passed as "develop" in tests). However, none of these instances include documentation that clarifies the intended purpose or behavior associated with this "version" (or environment) parameter.
• The parameter is used with the literal "develop" in tests, which suggests that it may trigger environment-specific logic.
• No inline comments or function-level documentation were found that describe what "develop" (or potentially other values) should affect in the router’s behavior.
It is recommended that the NewRouter function documentation be enhanced to clearly describe the role of the version/environment parameter to ensure correct usage and future maintainability.
| OperationID: "v2GetInfo", | ||
| OAuth2Scopes: []string{"ledger:read", "ledger:read"}, |
There was a problem hiding this comment.
Fix duplicate OAuth2 scopes.
The OAuth2 scopes array contains duplicate entries: "ledger:read" is listed twice.
Apply this diff to fix the duplicate scopes:
- OAuth2Scopes: []string{"ledger:read", "ledger:read"},
+ OAuth2Scopes: []string{"ledger:read"},📝 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.
| OperationID: "v2GetInfo", | |
| OAuth2Scopes: []string{"ledger:read", "ledger:read"}, | |
| OperationID: "v2GetInfo", | |
| OAuth2Scopes: []string{"ledger:read"}, |
2 participants
No description provided.