fix(v2.1): create transaction compatibility #696
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 modifies key build and processing components. The Earthfile now explicitly copies source files across all build stages, disables client SDK generation, and ensures the correct binary is included in the final image. Utility functions have been refactored to use pointer types and precise type handling, with corresponding test cases added. Ledger store queries now support PIT and date filtering with improved error handling, while new tests validate account and balance retrieval logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant EF as Earthfile
participant BS as Build System
Dev->>EF: Trigger build process
EF->>EF: Copy source files to /src for stages (generate, compile, tests, deploy, lint, bench)
EF-->>EF: Skip generate-client stage (disabled)
EF->>BS: Initiate build-image stage with compiled binary
BS-->>Dev: Final image built with correct binary
sequenceDiagram
participant App as Client/Application
participant Store as Ledgerstore
participant DB as Database
App->>Store: Call GetBalance(ctx, address, asset)
Store->>DB: Execute SQL query on MovesTable
alt Result found
DB-->>Store: Return balance result
else Not found (sqlutils.ErrNotFound)
DB-->>Store: Return error indicating missing record
Store-->>Store: Set balance to zero
end
Store-->>App: Return computed balance
Possibly related PRs
Suggested labels
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/script_test.go (1)
19-47: Add test cases for edge cases and error scenarios.The current test cases cover basic float64 and big integer conversions, but consider adding cases for:
- Negative numbers
- Zero values
- Numbers with decimal places
- Invalid string formats
Here's a suggested expansion of the test cases:
testCases := []testCase{ // ... existing cases ... + { + name: "negative number", + inputVars: map[string]any{ + "amount": map[string]any{ + "asset": "USD", + "amount": float64(-1000), + }, + }, + expected: map[string]string{ + "amount": "USD -1000", + }, + }, + { + name: "zero value", + inputVars: map[string]any{ + "amount": map[string]any{ + "asset": "USD", + "amount": float64(0), + }, + }, + expected: map[string]string{ + "amount": "USD 0", + }, + }, + { + name: "decimal number", + inputVars: map[string]any{ + "amount": map[string]any{ + "asset": "USD", + "amount": float64(100.50), + }, + }, + expected: map[string]string{ + "amount": "USD 100", + }, + }, + { + name: "invalid string format", + inputVars: map[string]any{ + "amount": map[string]any{ + "asset": "USD", + "amount": "not_a_number", + }, + }, + expected: map[string]string{ + "amount": "USD not_a_number", + }, + }, }internal/storage/ledgerstore/accounts_test.go (1)
190-201: Consider expanding test coverage for PIT filtering.While the test case effectively validates negative balance filtering with PIT, consider adding test cases for:
- Positive balance filtering with PIT
- Zero balance filtering with PIT
- Multiple PIT values to verify boundary conditions
Example test case:
+ t.Run("list using filter on positive balances and pit", func(t *testing.T) { + t.Parallel() + accounts, err := store.GetAccountsWithVolumes(ctx, NewGetAccountsQuery(NewPaginatedQueryOptions(PITFilterWithVolumes{ + PITFilter: PITFilter{ + PIT: pointer.For(now.Add(100 * time.Millisecond)), + }, + }). + WithQueryBuilder(query.Gt("balance[USD]", 0)), + )) + require.NoError(t, err) + require.Len(t, accounts.Data, 1) // account:1 + require.Equal(t, "account:1", accounts.Data[0].Account.Address) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docker-compose.ymlis excluded by!**/*.yml
📒 Files selected for processing (8)
Earthfile(1 hunks)internal/api/v2/utils.go(1 hunks)internal/script.go(1 hunks)internal/script_test.go(1 hunks)internal/storage/ledgerstore/accounts.go(2 hunks)internal/storage/ledgerstore/accounts_test.go(3 hunks)internal/storage/ledgerstore/balances.go(1 hunks)internal/storage/ledgerstore/balances_test.go(1 hunks)
🔇 Additional comments (10)
internal/script.go (1)
35-40: Verify potential precision loss in float64 to int conversion.The type switch handles both string and float64 amounts, but the float64 to int conversion could lead to precision loss for large numbers or numbers with decimal places.
Please confirm:
- Is precision loss acceptable in this context?
- Should we add validation to ensure the float64 value is within safe integer bounds?
Consider adding validation before the conversion:
case float64: + if amount > float64(math.MaxInt64) || amount < float64(math.MinInt64) { + // Handle overflow case + } s.Script.Vars[k] = fmt.Sprintf("%s %d", v["asset"], int(amount))internal/script_test.go (1)
49-63: LGTM! Well-structured test implementation.The test implementation is clean and follows best practices:
- Uses table-driven tests for better maintainability
- Enables parallel execution for better performance
- Uses clear test case names
- Properly validates the conversion results
internal/api/v2/utils.go (1)
50-58: LGTM! Improved pointer handling for PIT filter.The changes improve memory efficiency and make the code more idiomatic by using pointer types for optional time values.
internal/storage/ledgerstore/balances.go (2)
151-158: LGTM! Improved balance query implementation.The new implementation:
- Uses direct SQL query instead of function call, improving maintainability
- Correctly calculates balance using post-commit volumes
- Properly orders results by sequence
160-165: LGTM! Added proper handling for non-existent accounts.The changes ensure that non-existent accounts return a zero balance instead of an error, improving API compatibility.
internal/storage/ledgerstore/balances_test.go (1)
21-31: LGTM! Good test coverage for non-existent accounts.The test verifies that querying a non-existent account returns a zero balance without error, ensuring proper API behavior.
Earthfile (1)
122-122:❓ Verification inconclusive
Verify impact of disabling client generation.
The client generation has been disabled. Please ensure this won't impact users who depend on the generated client SDK.
🏁 Script executed:
#!/bin/bash # Check for any references to generated client code rg -l "github.com/formancehq/ledger/pkg/client"Length of output: 47
Manual Verification Required: Client SDK Impact
- The build command disabling client generation (
#BUILD +generate-client) is now in effect.- Our automated search using
rg -l "github.com/formancehq/ledger/pkg/client"returned no matches, suggesting there are no direct references to the generated client code.- However, given the low-confidence inference from the automated script, please manually verify that disabling client generation does not adversely affect any users dependent on the client SDK. Also, review tests, documentation, or other configurations that might reference the client SDK.
internal/storage/ledgerstore/accounts.go (3)
110-118: LGTM! Proper handling of PIT filtering for balances.The implementation correctly handles date filtering for balance queries with proper SQL injection prevention through parameterized queries.
127-134: LGTM! Consistent implementation of PIT filtering.The implementation maintains consistency with asset-specific balance filtering while ensuring secure query construction.
152-153: LGTM! Clean implementation of first_usage filtering.The implementation elegantly handles first_usage filtering using standard comparison operators.
…with more than 7 digits
gfyrag
force-pushed
the
hotfix/v2.1/create-transaction-compatiblity
branch
from
7a17f8f to
fd89662
Compare
No description provided.