fix: import from 2.1 #688
fix: import from 2.1 #688
Conversation
WalkthroughThe pull request converts identifier fields (ID) from direct integers to pointer types across multiple modules. The update adds new Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Test Suite
participant Import as Import Endpoint
participant Ledger as Ledger Engine
participant DB as Database
Tester->>Import: Send import request with ledger and JSON logs
Import->>Ledger: Process import request (parse logs, set pointer-based IDs)
Ledger->>DB: Insert logs and transactions with dereferenced IDs
DB-->>Ledger: Confirm insertion
Ledger-->>Import: Return import success
Import-->>Tester: Respond with imported ledger data
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (23)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (22)
⏰ Context from checks skipped due to timeout of 90000ms (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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #688 +/- ##
==========================================
- Coverage 81.61% 81.55% -0.07%
==========================================
Files 131 131
Lines 7071 7085 +14
==========================================
+ Hits 5771 5778 +7
- Misses 996 1004 +8
+ Partials 304 303 -1 ☔ View full report in Codecov by Sentry. |
![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 (9)
internal/api/v1/controllers_transactions.go (1)
20-21: LGTM! Consider adding documentation for pointer fields.The changes correctly update the ID fields to pointer types. Consider adding documentation to explain why these fields are pointers and the significance of the json:"-" tag on the ID field.
type Ret struct { Aux Reverted bool `json:"reverted"` PreCommitVolumes ledger.PostCommitVolumes `json:"preCommitVolumes,omitempty"` PreCommitEffectiveVolumes ledger.PostCommitVolumes `json:"preCommitEffectiveVolumes,omitempty"` + // TxID is a pointer to allow for null values in the JSON response TxID *int `json:"txid"` + // ID is excluded from JSON serialization as it's used internally ID *int `json:"-"` }test/e2e/api_ledgers_import_test.go (2)
56-60: Verify test data integrity.The test data contains transactions with sequential IDs starting from 0, which might not reflect real-world scenarios where IDs could have gaps or start from different values. Consider adding test cases with non-sequential IDs to ensure robust handling of imported data.
68-105: Enhance test coverage for edge cases.While the test verifies basic import functionality, consider adding test cases for:
- Invalid JSON format
- Missing required fields
- Inconsistent hash values
- Out-of-order transactions
internal/controller/ledger/controller_default.go (1)
208-208: Add nil checks before dereferencing IDs.The code dereferences pointer IDs without checking for nil values. While these IDs are likely to be non-nil in the current implementation, adding nil checks would make the code more robust against future changes.
Apply this pattern for safer dereferencing:
-return fmt.Errorf("importing log %d: %w", *log.ID, err) +if log.ID == nil { + return fmt.Errorf("importing log with nil ID: %w", err) +} +return fmt.Errorf("importing log %d: %w", *log.ID, err)Also applies to: 220-220, 224-224, 233-233, 276-276, 283-283
internal/storage/ledger/transactions_test.go (1)
561-562: Add error handling test cases for nil IDs.While the current test cases cover the happy path, consider adding test cases that verify proper error handling when transaction IDs are nil.
Also applies to: 573-574, 712-713
internal/README.md (4)
478-479: Updated Log ID Field to Pointer TypeThe
IDfield in theLogstruct has been updated to use a pointer (*int) instead of a direct integer. Please verify that this change is consistently handled in serialization/deserialization and that the logic correctly checks for nil IDs where necessary.
520-524: Addition of Log.WithID MethodThe new
WithIDmethod for theLogtype allows setting an ID value and returning an updated copy. Ensure that its behavior is well documented—specifically, that the method converts the provided integer into a pointer and that consumers are aware that it returns a new instance rather than mutating the receiver.
847-848: Updated Transaction ID Field to Pointer TypeThe
IDfield in theTransactionstruct has been modified to be of type*int. Confirm that this change does not impact any logic that assumes the ID is always non-nil and that nullability is correctly handled in business rules and JSON marshaling.
931-935: Addition of Transaction.WithID MethodThe new
WithIDmethod for theTransactiontype provides a clean way to set the ID on a transaction. Please ensure that this method wraps the provided ID in a pointer properly and that its copy semantics align with other methods on this type. Adding clarifying documentation or examples in the README might be helpful for future users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (22)
internal/README.md(100 hunks)internal/api/bulking/bulker.go(4 hunks)internal/api/v1/controllers_transactions.go(1 hunks)internal/api/v1/controllers_transactions_create_test.go(3 hunks)internal/api/v2/controllers_bulk_test.go(12 hunks)internal/api/v2/controllers_transactions_read_test.go(1 hunks)internal/controller/ledger/controller_default.go(4 hunks)internal/controller/ledger/controller_default_test.go(4 hunks)internal/controller/ledger/log_process.go(1 hunks)internal/controller/ledger/log_process_test.go(2 hunks)internal/log.go(7 hunks)internal/storage/ledger/logs.go(1 hunks)internal/storage/ledger/logs_test.go(5 hunks)internal/storage/ledger/moves_test.go(5 hunks)internal/storage/ledger/paginator_column.go(1 hunks)internal/storage/ledger/resource.go(1 hunks)internal/storage/ledger/transactions.go(4 hunks)internal/storage/ledger/transactions_test.go(11 hunks)internal/transaction.go(2 hunks)internal/transaction_test.go(6 hunks)pkg/testserver/helpers.go(2 hunks)test/e2e/api_ledgers_import_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/storage/ledger/resource.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (26)
internal/api/v2/controllers_transactions_read_test.go (1)
30-30: Verify the test logic using transaction ID.The test should use the transaction's ID (
tx.ID) instead of a hardcoded value0to ensure the correct transaction is being queried. This would better validate the transaction retrieval logic.- Builder: query.Match("id", 0), + Builder: query.Match("id", tx.ID),internal/controller/ledger/log_process_test.go (1)
72-75: LGTM! Proper handling of ID as pointer type.The changes correctly implement the pointer-based ID handling using
pointer.For(0)to set the log ID.pkg/testserver/helpers.go (1)
39-39: LGTM! Proper conversion of SDK transaction ID to pointer type.The change correctly converts the SDK transaction ID to a pointer type using
pointer.For(int(tx.ID.Int64())).internal/storage/ledger/logs.go (1)
90-92: LGTM! The ID check has been correctly updated.The condition has been updated to check for
nilinstead of0, which aligns with the transition to pointer types for IDs.internal/controller/ledger/log_process.go (1)
80-80: LGTM! The ID is correctly dereferenced in the debug log.The log statement now properly dereferences the ID pointer to display the actual value.
internal/transaction_test.go (1)
81-81: LGTM! Test cases have been consistently updated to use pointer IDs.The changes correctly reflect the transition to pointer types for IDs:
- Using
WithID(0)for new transactions- Setting
log.IDusingpointer.For(0)Also applies to: 90-90, 99-99, 152-153, 162-163
internal/storage/ledger/moves_test.go (1)
62-62:❓ Verification inconclusive
Verify the hardcoded TransactionID values.
All moves (m1-m5) are now using a hardcoded
TransactionID: 0instead of referencingtx.ID. This could break the relationship between moves and their originating transactions. Please verify if this is intentional or if the moves should maintain their relationship with the transaction created at line 35.Run the following script to check how TransactionID is used in moves across the codebase:
Also applies to: 79-79, 96-96, 113-113, 130-130
🏁 Script executed:
#!/bin/bash # Description: Check how TransactionID is used in moves # Search for TransactionID usage in move-related code rg -A 5 'TransactionID.*Move' # Search for test cases involving moves and transactions rg -A 5 'TestMove.*Transaction'Length of output: 65
Manual Verification Required: Confirm Transaction Relationship Handling
The automated search did not reveal any references connecting moves to dynamic transaction IDs (e.g., via
tx.ID). This inconclusive output means we cannot confirm if using a hardcodedTransactionID: 0ininternal/storage/ledger/moves_test.go(at lines 62, 79, 96, 113, and 130) is intentional. Please manually verify whether the moves’ association with their originating transaction—created around line 35—should instead use the actual transaction ID.internal/api/v1/controllers_transactions_create_test.go (2)
5-5: LGTM!The import of the
pointerpackage is necessary for handling pointer-based IDs.
224-224: LGTM!The changes correctly update the test to use pointer-based IDs, maintaining consistency with the new ID handling approach.
Also applies to: 234-236
internal/storage/ledger/paginator_column.go (1)
226-229: LGTM!The addition of pointer type handling for
int64andintis well-implemented and necessary for supporting the new pointer-based ID fields.internal/storage/ledger/logs_test.go (3)
43-43: LGTM!The test assertions are correctly updated to dereference the pointer-based IDs before comparison.
Also applies to: 61-61
133-135: LGTM!The hash consistency test is properly updated to handle pointer-based IDs.
218-218: LGTM!The pagination test assertions are correctly updated to dereference pointer-based IDs.
Also applies to: 226-226, 241-241
internal/transaction.go (2)
42-42: LGTM!The conversion of the
IDfield to a pointer type is well-implemented and aligns with the broader effort to standardize ID handling.
66-69: LGTM!The
WithIDmethod is a clean implementation for setting pointer-based IDs, following the builder pattern consistently used throughout the codebase.internal/api/bulking/bulker.go (1)
144-144:❓ Verification inconclusive
LGTM! Correctly dereferencing log IDs.
The changes correctly dereference the log IDs across all action handlers in the
processElementmethod, aligning with the transition to pointer-based ID fields.Run the following script to verify that all log ID dereferences are consistent:
Also applies to: 186-186, 203-203, 247-247
🏁 Script executed:
#!/bin/bash # Description: Verify that all log ID dereferences are consistent. # Test: Search for log.ID usage. Expect: All occurrences should be dereferenced. rg -A 2 'log\.ID' | grep -v '\*log\.ID'Length of output: 41
Manual verification required: Validate log ID dereferences across action handlers
The changes in
internal/api/bulking/bulker.go(lines 144, 186, 203, and 247) appear to correctly dereference thelog.IDfield (e.g., using*log.ID). However, the automated check using:rg -A 2 'log\.ID' | grep -v '\*log\.ID'did not produce output, which is inconclusive. Please ensure through manual verification that every instance of
log.IDin the codebase is properly dereferenced.internal/storage/ledger/transactions.go (3)
77-77: LGTM! Correctly dereferencing transaction IDs in moves.The changes correctly dereference the transaction IDs when setting the
TransactionIDfield in the moves.Also applies to: 89-89
121-123: LGTM! Correctly handling nil transaction IDs.The condition now correctly checks for nil transaction IDs before generating a new ID.
142-143: LGTM! Correctly dereferencing transaction ID in trace attributes.The change correctly dereferences the transaction ID when setting trace attributes.
internal/log.go (4)
94-94: LGTM! Correctly updated ID field type.The ID field is correctly changed to a pointer type, allowing for nullable IDs.
107-109: LGTM! Correctly setting ID in ChainLog.The changes correctly use
pointer.Forto set the ID in theChainLogmethod.
162-167: LGTM! Correctly handling nil IDs in ComputeHash.The changes correctly handle nil IDs when computing the hash.
176-179: LGTM! Added WithID method.The new
WithIDmethod provides a clean way to set the ID field.internal/controller/ledger/controller_default_test.go (1)
61-64: LGTM! Correctly setting log IDs in test mocks.The changes correctly set log IDs using
pointer.Forin all test mocks, ensuring consistency with the pointer-based ID fields.Also applies to: 110-114, 153-157, 194-198
internal/api/v2/controllers_bulk_test.go (1)
72-74: LGTM! Consistent use of pointer.For() for ID fields.The changes correctly implement pointer-based ID handling, ensuring consistency across the codebase.
Also applies to: 125-127, 155-157
internal/storage/ledger/transactions_test.go (1)
240-241: LGTM! Correct assertions for pointer-based IDs.The test assertions correctly handle the pointer-based IDs by dereferencing them before comparison.
Also applies to: 262-263, 288-289
gfyrag
force-pushed
the
fix/import-from-2.1
branch
from
c32cdf8 to
7b49031
Compare
No description provided.