fix(ledger): tx reversion handles empty middle accounts #711
Conversation
WalkthroughThis change updates the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Transaction Caller
participant C as DefaultController
participant B as Balance Map
Client->>C: revertTransaction(tx)
alt Non-forced Transaction Reversal
C->>B: Retrieve current balance for posting.Destination
C->>B: Add posting.Amount to current balance
end
C-->>Client: Transaction reverted with updated balances
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
CrosleyZack
force-pushed
the
crosley/ledger-fix-tx-reversion
branch
from
3660f41 to
3b32fc5
Compare
| if _, ok := balances[posting.Destination]; ok { | ||
| // if this source is also a destination | ||
| balances[posting.Destination][posting.Asset] = balances[posting.Destination][posting.Asset].Add( | ||
| balances[posting.Destination][posting.Asset], | ||
| posting.Amount, | ||
| ) | ||
| } |
There was a problem hiding this comment.
If the destination is also a source, which it should only appear in balances if it is (see lines 388-390), then add the posting amount into the tracked balance.
CrosleyZack
force-pushed
the
crosley/ledger-fix-tx-reversion
branch
from
3b32fc5 to
667e480
Compare
gfyrag
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
==========================================
+ Coverage 81.49% 81.70% +0.20%
==========================================
Files 133 135 +2
Lines 7170 7290 +120
==========================================
+ Hits 5843 5956 +113
- Misses 1023 1027 +4
- Partials 304 307 +3 ☔ View full report in Codecov by Sentry. |
CrosleyZack
force-pushed
the
crosley/ledger-fix-tx-reversion
branch
from
667e480 to
6f0046f
Compare
![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 (1)
test/e2e/api_transactions_revert_test.go (1)
222-283: Test case successfully validates the fix for empty passthrough accounts.The new test scenario correctly validates the expected behavior of reverting transactions through empty passthrough accounts. This aligns with the PR objective of fixing transactions involving accounts with zero balances.
A few suggestions for improvement:
- For consistency with other test blocks, consider moving the
RevertTransactioncall inside aBeforeEachblock.- Consider adding balance verification after reversion to fully validate that all accounts return to their original state.
When("trying to revert the passthrough transaction", func() { - _, err := RevertTransaction( - ctx, - testServer.GetValue(), - operations.V2RevertTransactionRequest{ - Ledger: "default", - ID: tx.ID, - AtEffectiveDate: pointer.For(true), - }, - ) - Expect(err).To(Succeed()) + BeforeEach(func() { + _, err := RevertTransaction( + ctx, + testServer.GetValue(), + operations.V2RevertTransactionRequest{ + Ledger: "default", + ID: tx.ID, + AtEffectiveDate: pointer.For(true), + }, + ) + Expect(err).To(Succeed()) + }) + + It("should restore original account balances", func() { + // Add balance checks for alice, bob, and world accounts + // to verify they return to their original state + }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/controller/ledger/controller_default.go(1 hunks)test/e2e/api_transactions_revert_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/ledger/controller_default.go
CrosleyZack
force-pushed
the
crosley/ledger-fix-tx-reversion
branch
from
6f0046f to
674744e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/api_transactions_revert_test.go (1)
229-257: Consider adding additional assertions for account balances.While the test validates that the transaction is reverted, it doesn't explicitly verify that the account balances are correctly restored after reversion, which is the core issue being fixed. Consider adding balance checks before and after reversion.
Add balance verification to fully validate the fix:
// Add after transaction creation balanceWalter, err := GetAccount(ctx, testServer.GetValue(), operations.V2GetAccountRequest{ Ledger: "default", Address: "walter", }) Expect(err).NotTo(HaveOccurred()) Expect(balanceWalter.Balances["USD"]).To(Equal("-100")) // Walter sent 100 USD balanceWendy, err := GetAccount(ctx, testServer.GetValue(), operations.V2GetAccountRequest{ Ledger: "default", Address: "wendy", }) Expect(err).NotTo(HaveOccurred()) Expect(balanceWendy.Balances["USD"]).To(Equal("0")) // Wendy received and sent 100 USD // Add after transaction reversion balanceWalterAfter, err := GetAccount(ctx, testServer.GetValue(), operations.V2GetAccountRequest{ Ledger: "default", Address: "walter", }) Expect(err).NotTo(HaveOccurred()) Expect(balanceWalterAfter.Balances["USD"]).To(Equal("0")) // Balance restored balanceWendyAfter, err := GetAccount(ctx, testServer.GetValue(), operations.V2GetAccountRequest{ Ledger: "default", Address: "wendy", }) Expect(err).NotTo(HaveOccurred()) Expect(balanceWendyAfter.Balances["USD"]).To(Equal("0")) // Still zero
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/controller/ledger/controller_default.go(1 hunks)test/e2e/api_transactions_revert_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/ledger/controller_default.go
🔇 Additional comments (1)
test/e2e/api_transactions_revert_test.go (1)
222-283: Well-structured test case for passthrough account scenario.This new test case effectively validates the fix for handling transaction reversion through an empty passthrough account. The test creates a transaction where "wendy" acts as both a destination and source account in different postings, which directly addresses the issue described in the PR objectives.
A few observations:
- The test correctly creates a transaction with two linked postings (walter → wendy → world)
- It verifies that the transaction can be reverted properly
- The assertions check both that the transaction is marked as reverted and that the timestamp is preserved
This test provides good coverage for the scenario where an intermediate account with zero balance is involved in a transaction that needs to be reverted.
CrosleyZack
force-pushed
the
crosley/ledger-fix-tx-reversion
branch
3 times, most recently
from
65fc253 to
cabdc82
Compare
CrosleyZack
force-pushed
the
crosley/ledger-fix-tx-reversion
branch
from
cabdc82 to
b5dd7c3
Compare
|
Thank you @CrosleyZack! |
Suppose we have three accounts:
account_onewith balance 10account_twowith balance 0worldIf we create a transaction with postings
account_one -5-> account_twoaccount_two -5-> worldwe will end up with final balances
account_onewith balance 5account_twowith balance 0worldWhen trying to revert this transaction, we will:
account_oneandaccount_two[line 388]worldandaccount_two[line 395]posting.Amountfrom each ofworldandaccount 2[line 405]In this example, because
account_twohas no balance we will deduct5credits from it and end up with-5, thus the reversion is failed. However, this only occurs because we account for the credits being removed fromaccount_twoand not the credits being added.The solution is to track both the source and destination when adjusting credits on line 405 so we can add in the five credits that will be added to
account_twofromworldso they exist in order to be deducted via the reversion