fix: invalid pcv #831
fix: invalid pcv #831
Conversation
WalkthroughThis PR introduces multiple updates to SQL migration scripts. A new JSON entry is added to the postings array in one test file. Several migration scripts update the transaction identifier from Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as Procedure
participant TV as Temporary Table (moves_view)
participant TT as Transactions Table
participant Notif as Notification System
Proc->>TV: Drop existing moves_view and create new one
Proc->>TV: Create index on transactions_seq
Proc->>TV: Check for records
alt moves_view is empty
Proc->>Notif: Send exit notification
Proc->>Proc: Exit procedure
else moves_view has records
loop For each batch
Proc->>TV: Select batch of records
Proc->>TT: Update transactions with aggregated JSON data
Proc->>Notif: Send progress notification
end
end
Proc->>TV: Drop moves_view
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #831 +/- ##
==========================================
+ Coverage 81.92% 81.97% +0.05%
==========================================
Files 137 137
Lines 7512 7512
==========================================
+ Hits 6154 6158 +4
+ Misses 1040 1038 -2
+ Partials 318 316 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (5)
internal/storage/bucket/migrations/19-transactions-fill-pcv/up_tests_after.sql (1)
2-3: Verify Expected JSON UpdateThe updated
expectedvariable now includes the additional"SELL"key for both the"world"and"sellers:0"objects. Please confirm that this new JSON structure is fully in line with the corresponding schema and test expectations introduced in linked migration scripts.internal/storage/bucket/migrations/0-init-schema/up_tests_after.sql (1)
17-22: Verify New "SELL" Posting EntryA new posting entry for the
"SELL"asset has been inserted into the JSON array. The snippet correctly constructs the object with:
"destination": "sellers:" || (seq % 5)"source": "world""asset": "SELL""amount": 1Ensure that this aligns with the updated test expectations and the related changes in the schema.
internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql (1)
10-11: Validate Aggregation Logic for Transaction VolumesThe creation of the temporary table
moves_viewnow usestransactions_seqwith aggregation viapublic.aggregate_objectsand JSON operations. Verify that castingtransactions_seqto numeric (as seen in the inner query) is required and that it correctly supports downstream operations.internal/storage/bucket/migrations/27-fix-invalid-pcv/up.sql (1)
10-11: Validate Aggregation Logic for PCVThe temporary table
moves_viewis again built usingtransactions_seqand the aggregation function. Ensure that the JSON construction (viajsonb_build_object) and conversion to numeric are returning the expected format for downstream PCV calculations.internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql (1)
1-7: New Migration for PCV Missing AssetThis new migration script addresses the missing asset issue in the PCV calculations. It constructs the temporary table
moves_viewby aggregating volume data and, importantly, utilizesjson_build_object(asset, json_build_object(...))to ensure the asset is included. Please confirm that this change fully resolves the missing asset problem in related PCV logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
internal/storage/bucket/migrations/28-fix-pcv-missing-asset/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/29-accounts-recreate-unique-index/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/30-clean-database/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/31-logs-async-hash-procedure/notes.yamlis excluded by!**/*.yaml
📒 Files selected for processing (5)
internal/storage/bucket/migrations/0-init-schema/up_tests_after.sql(1 hunks)internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql(3 hunks)internal/storage/bucket/migrations/19-transactions-fill-pcv/up_tests_after.sql(1 hunks)internal/storage/bucket/migrations/27-fix-invalid-pcv/up.sql(3 hunks)internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (10)
internal/storage/bucket/migrations/19-transactions-fill-pcv/up_tests_after.sql (1)
9-9: PL/pgSQL Block Terminator CheckThe end delimiter (
$$) marked with a change appears correctly placed. Just ensure that any formatting or minor changes around the closing block do not affect execution.internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql (4)
15-20: Ensure Consistency in Transaction Sequence HandlingThe inner query uses
DISTINCT ON (moves.transactions_seq, accounts_address, asset)together with a window function to pick the latestpost_commit_volumes. This change—migrating from the old identifier totransactions_seq—appears correct. Just double-check that all parts of the system now consistently refer totransactions_seq.
29-29: Index Creation on Transaction SequenceThe index on
moves_view(transactions_seq)supports the new query pattern and the updated foreign key. This change is appropriate for maintaining performance.
37-47: Review Batch Update LogicIn the loop, the update statement matches rows via
where transactions.seq = data.transactions_seq. Please verify that the fieldtransactions.seqis the correct reference for the new transaction identifier and that the batching logic (using_offsetand_batch_size) correctly processes all records.
60-63: Alter Table Constraint AdditionAdding the
post_commit_volumes_not_nullconstraint at the end appears safe. Confirm that the data updated during the batch process meets this constraint and that thenot validclause is intentional (perhaps for a later validation phase).internal/storage/bucket/migrations/27-fix-invalid-pcv/up.sql (3)
15-20: Confirm Transaction Sequence GroupingThe inner query now groups by
moves.transactions_seq,accounts_address, andassetusingDISTINCT ONand window functions. This logic appears to be designed to extract the latest posting values—please confirm that this meets the desired data semantics.
29-29: Indexing VerificationCreating an index on
moves_view(transactions_seq)is consistent with recent changes and should help query performance during the update phase.
37-47: Review Batch Processing UpdateThe loop for batch processing uses the updated
transactions_seqto select and update data. Verify that the join conditionwhere transactions.seq = data.transactions_seqcorrectly maps to the new schema and that no legacy references remain.internal/storage/bucket/migrations/28-fix-pcv-missing-asset/up.sql (2)
15-26: Check Aggregation Query for Asset VolumesThe inner query groups by
transactions_seqandaccounts_addresswhile employing aDISTINCT ONclause and window function to correctly extract the latestpost_commit_volumes. Verify that this updated aggregation accurately reflects the intended per-asset volume breakdown.
37-47: Review Batch Processing LoopThe batch update loop is consistent with other migration scripts, updating the
transactionstable using the newly aggregated volumes. Please ensure that the join condition involvingtransactions.seqmatches thetransactions_seqfield and that the batching logic performs as expected.
No description provided.