feat: avoid tracking unbounded balances #1665
Conversation
WalkthroughThe changes involve significant modifications to the account management and locking mechanisms within the ledger's virtual machine and compiler components. Key updates include the introduction of new fields for tracking read and write lock accounts, enhancements to error handling, and improvements to the parsing and compilation processes. These adjustments aim to optimize the handling of overdrafts and streamline resource management during transactions. Changes
Assessment against linked issues
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 (
|
ascandone
force-pushed
the
perf/do-no-lock-unbounded-sources
branch
from
e6911d3
to
1c3e608
Compare
|
|
||
| return involvedAccounts, involvedSources, nil | ||
| }() | ||
| readLockAccounts, writeLockAccounts, err := m.ResolveResources(ctx, commander.store) |
There was a problem hiding this comment.
Note: this is the only usage of the ResolveResources method
| neededBalances: make(map[machine.Address]map[machine.Address]struct{}), | ||
| sources: map[machine.Address]struct{}{}, | ||
| writeLockAccounts: map[machine.Address]struct{}{}, | ||
| readLockAccounts: map[machine.Address]struct{}{}, |
There was a problem hiding this comment.
here the diff is weird because of golang's formatting, but the actual diff is this:
+ writeLockAccounts: map[machine.Address]struct{}{},
+ readLockAccounts: map[machine.Address]struct{}{},
}
| Resources: visitor.resources, | ||
| NeededBalances: visitor.neededBalances, | ||
| Sources: sources, | ||
| Instructions: visitor.instructions, |
There was a problem hiding this comment.
actual diff:
Instructions: visitor.instructions,
Resources: visitor.resources,
NeededBalances: visitor.neededBalances,
- Sources: sources,
+ReadLockAccounts: readLockAccounts,
+WriteLockAccounts: writeLockAccounts,
| } | ||
| } | ||
| return nil, fmt.Errorf("missing %v balance from %v", mon.Asset, account) | ||
|
|
||
| return &machine.Funding{ |
There was a problem hiding this comment.
Instead of emitting the error when the account/asset balance is not found, return the required needed amount.
This is what allows unbounded accounts to send without locking their balance
| @@ -607,16 +611,19 @@ func (m *Machine) ResolveResources(ctx context.Context, store Store) ([]string, | |||
| m.Resources = append(m.Resources, val) | |||
| } | |||
|
|
|||
| involvedAccounts := make([]string, 0) | |||
| involvedSources := make([]string, 0) | |||
| for _, accountAddress := range involvedAccountsMap { | |||
There was a problem hiding this comment.
Instead of marking every possible account that appears within the program (directly or indirectly - e.g. the result of meta()) as readLock account, we only iterate through m.Program.ReadLockAccounts
| @@ -528,6 +529,59 @@ func TestWorldSource(t *testing.T) { | |||
| test(t, tc) | |||
| } | |||
|
|
|||
| func TestUnboundedSourceSimple(t *testing.T) { | |||
There was a problem hiding this comment.
The purpose of these tests is to cover the case in which something is sent from an account whose balance was not given (the tests were red before changing the compiler output)
| } | ||
| test(t, tc) | ||
| } | ||
|
|
||
| func TestNoEmptyPostings(t *testing.T) { |
There was a problem hiding this comment.
The purpose of these tests is to assert against the value of readLockAccounts, writeLockAccounts, and to verify which balances are required by store.GetBalance()
Please have a very careful look at those specs before approving
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- components/ledger/internal/engine/command/commander.go (2 hunks)
- components/ledger/internal/machine/script/compiler/compiler.go (4 hunks)
- components/ledger/internal/machine/script/compiler/compiler_test.go (2 hunks)
- components/ledger/internal/machine/script/compiler/destination.go (2 hunks)
- components/ledger/internal/machine/script/compiler/source.go (3 hunks)
- components/ledger/internal/machine/vm/machine.go (4 hunks)
- components/ledger/internal/machine/vm/machine_test.go (5 hunks)
- components/ledger/internal/machine/vm/program/program.go (1 hunks)
Files skipped from review due to trivial changes (1)
- components/ledger/internal/machine/script/compiler/compiler_test.go
Additional comments not posted (11)
components/ledger/internal/machine/vm/program/program.go (1)
17-18: Structural changes toProgramare appropriate for the new locking strategy.The addition of
ReadLockAccountsandWriteLockAccountsaligns with the PR's objectives to optimize locking mechanisms. Ensure these fields are properly initialized and used effectively throughout the codebase.The structural changes are approved.
Run the following script to verify the initialization and usage of the new fields:
Verification successful
Initialization and usage of
ReadLockAccountsandWriteLockAccountsare verified.The fields are properly initialized in the
compilerpackage and effectively used in themachinepackage, aligning with the intended locking strategy.
- Initialization:
components/ledger/internal/machine/script/compiler/compiler.go- Usage:
components/ledger/internal/machine/vm/machine.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of `ReadLockAccounts` and `WriteLockAccounts`. # Test: Search for the initialization and usage of the new fields. Expect: Proper initialization and usage. rg --type go -A 5 $'ReadLockAccounts|WriteLockAccounts'Length of output: 2138
components/ledger/internal/machine/script/compiler/destination.go (1)
Line range hint
26-37: Enhancements to destination address handling are well-integrated.The introduction of
destAddrand conditional read lock management based on whether the address is part of the "world" enhances the method's functionality. Verify the integration and ensure there are no unintended side effects on the control flow.The changes to the method are approved.
Run the following script to verify the integration and side effects of the new logic:
Verification successful
Integration of
destAddrandisWorldLogic VerifiedThe integration of
destAddrand the conditional logic usingisWorldindestination.gois consistent with the rest of the codebase. The handling of the "world" account is correctly implemented, and there are no unintended side effects observed. The changes are well-integrated and align with existing patterns.
- The
isWorldfunction is used consistently across the codebase to handle special account types.- The logic for applying read locks based on
destAddris correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and side effects of the new logic in `VisitDestinationRecursive`. # Test: Search for the usage of `destAddr` and the conditional logic. Expect: Proper integration without side effects. rg --type go -A 5 $'destAddr|isWorld'Length of output: 4127
components/ledger/internal/machine/script/compiler/source.go (2)
102-119: Addition ofisOverdraftUnboundedmethod enhances overdraft handling.The method correctly identifies unbounded overdraft contexts, improving the robustness of the account processing flow. Verify the integration and ensure there are no unintended side effects on the control flow.
The addition of the method is approved.
Run the following script to verify the integration and side effects of the new logic:
Verification successful
Integration of
isOverdraftUnboundedmethod appears correct.The method is used in a conditional check to determine account handling based on overdraft status. Ensure that the related logic, including the
isWorldmethod, is functioning as expected to avoid unintended side effects.
- Verify the behavior of the
isWorldmethod and its interaction withisOverdraftUnbounded.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and side effects of the `isOverdraftUnbounded` method. # Test: Search for the usage of `isOverdraftUnbounded`. Expect: Proper integration without side effects. rg --type go -A 5 $'isOverdraftUnbounded'Length of output: 1156
Line range hint
151-187: Modifications toVisitSourcemethod improve overdraft management.The use of
isOverdraftUnboundedand adjustments to the instructions based on the account type enhance the method's functionality. Verify the integration and ensure there are no unintended impacts on the control flow.The changes to the method are approved.
Run the following script to verify the integration and impacts of the new logic:
Verification successful
Verification Successful: Integration of
isOverdraftUnboundedinVisitSourceis consistent.The changes to the
VisitSourcemethod, including the use ofisOverdraftUnbounded, are well-integrated and align with the intended functionality. The control flow modifications enhance the handling of overdrafts and world accounts without unintended impacts.
- The
isOverdraftUnboundedfunction is properly defined and used within the same file.- The control flow changes in
VisitSourceare consistent with the overall design.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and impacts of the changes in `VisitSource`. # Test: Search for the usage of `isOverdraftUnbounded` and related logic. Expect: Proper integration without unintended impacts. rg --type go -A 5 $'VisitSource|isOverdraftUnbounded'Length of output: 6330
components/ledger/internal/engine/command/commander.go (1)
128-134: Refactor and Error Handling Improvements inexecMethodThe changes to use
m.ResolveResourcesdirectly and the new error handling withNewErrCompilationFailedsimplify the control flow and improve error handling consistency. However, the removal ofworldFiltershould be carefully reviewed to ensure it doesn't inadvertently affect other functionalities.The code changes are approved.
Run the following script to verify the impact of removing
worldFilter:Verification successful
Removal of
worldFilterVerifiedThe removal of
worldFilterdoes not affect other functionalities, as there are no remaining references in the codebase, including tests and documentation. This confirms that the change was clean and should not introduce any issues.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the removal of `worldFilter` affects other functionalities. # Test: Search for any remaining references to `worldFilter`. Expect: No occurrences. rg --type go 'worldFilter'Length of output: 26
Script:
#!/bin/bash # Description: Verify if the removal of `worldFilter` affects tests or documentation. # Test: Search for any references to `worldFilter` in test files. rg --type go 'worldFilter' --glob '*_test.go' # Test: Search for any references to `worldFilter` in documentation files. rg 'worldFilter' --glob '*.md'Length of output: 76
components/ledger/internal/machine/vm/machine.go (2)
146-152: Improved Reliability inwithdrawAlwaysMethodThe modification to consistently return a
machine.Fundingobject enhances reliability and ensures predictable behavior, especially for unbounded accounts.The code changes are approved.
614-626: Code Simplification and Enhanced Output Consistency inResolveResourcesMethodThe direct population of
readLockAccountsandwriteLockAccountsfrom them.Programstructure, along with sorting the accounts before returning, simplifies the code and enhances output consistency. This is crucial for the correct management of account locks during transactions.The code changes are approved.
components/ledger/internal/machine/script/compiler/compiler.go (1)
31-37: Enhancements to Account Locking Mechanisms inparseVisitorStruct andVisitVarsMethodThe addition of
writeLockAccountsandreadLockAccountsfields, along with the modification in theVisitVarsmethod to include an assignment toreadLockAccounts, enhances the functionality by providing a clearer delineation of account access types. This is crucial for maintaining the integrity of operations involving account balances and transactions.The code changes are approved.
Also applies to: 592-592
components/ledger/internal/machine/vm/machine_test.go (3)
532-552: Review: TestUnboundedSourceSimpleThis test case is designed to validate the handling of transactions from an unbounded source, ensuring that the system correctly processes such transactions without requiring balance checks. The implementation appears correct and aligns with the PR's objectives to optimize performance by avoiding unnecessary balance locks.
The test case is well-implemented and serves its purpose effectively.
554-583: Review: TestUnboundedSourceInorderThis test case checks the transaction processing when multiple sources are involved, including an unbounded source. It ensures that the system handles the order of sources correctly and that the unbounded source does not affect the processing of bounded sources. The test is crucial for verifying the robustness of the new transaction handling logic.
The test case is correctly implemented and tests the intended functionality.
2342-2358: Review: mockStore ImplementationThe
mockStoretype and its methods are crucial for simulating the store behavior in tests without relying on a full store implementation. This mock store tracks requested accounts and simulates balance retrieval, which is essential for testing the new locking logic.
- The
GetRequestedAccountsmethod sorts the accounts before returning them, which is good for consistent test results.- The
GetBalancemethod correctly simulates a balance retrieval by always returning zero, which is sufficient for the purpose of these tests.- The
GetAccountmethod is not implemented and panics, which is acceptable in this context since it's not used by the current test cases.The mock store is appropriately implemented for the current testing needs.
ascandone
force-pushed
the
perf/do-no-lock-unbounded-sources
branch
from
c4f59ef
to
8494a04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
components/ledger/internal/machine/vm/machine_test.go (2)
Line range hint
981-1022: LGTM: Updated test case for balance tracking with different account types.The modifications correctly test the balance tracking behavior for different account types, including world, bounded overdraft, and unbounded overdraft accounts. This aligns well with the new requirements.
Consider adding a comment explaining the expected behavior for each account type to improve readability and maintainability. For example:
send [GEM 15] ( source = { + // Normal accounts should be tracked $a @b + // World account should not be tracked (unbounded) max [GEM 1] from @world + // Bounded overdraft accounts should be locked @bounded allowing overdraft up to [GEM 1] + // Unbounded overdraft accounts should not be locked @unb allowing unbounded overdraft } destination = { max [GEM 1] to @c remaining to @world } )
1047-1071: LGTM: New test case for balance tracking withbalance()function.This test case effectively verifies that accounts used in the
balance()function are properly tracked and locked. It ensures that both the source account and the account whose balance is queried are included in the appropriate lock lists.Consider adding assertions for the specific lock types (read/write) for each account to make the test more explicit:
require.Equal(t, []string{"a"}, wlAccounts) require.Equal(t, []string{"acc", "b"}, rlAccounts) + +// Assert specific lock types +require.True(t, slices.Contains(wlAccounts, "a"), "Expected 'a' to have a write lock") +require.True(t, slices.Contains(rlAccounts, "acc"), "Expected 'acc' to have a read lock") +require.True(t, slices.Contains(rlAccounts, "b"), "Expected 'b' to have a read lock")
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- components/ledger/internal/engine/command/commander.go (2 hunks)
- components/ledger/internal/machine/script/compiler/compiler.go (4 hunks)
- components/ledger/internal/machine/script/compiler/compiler_test.go (2 hunks)
- components/ledger/internal/machine/script/compiler/destination.go (2 hunks)
- components/ledger/internal/machine/script/compiler/source.go (3 hunks)
- components/ledger/internal/machine/vm/machine.go (4 hunks)
- components/ledger/internal/machine/vm/machine_test.go (5 hunks)
- components/ledger/internal/machine/vm/program/program.go (1 hunks)
Files skipped from review due to trivial changes (1)
- components/ledger/internal/machine/script/compiler/compiler_test.go
Files skipped from review as they are similar to previous changes (6)
- components/ledger/internal/engine/command/commander.go
- components/ledger/internal/machine/script/compiler/compiler.go
- components/ledger/internal/machine/script/compiler/destination.go
- components/ledger/internal/machine/script/compiler/source.go
- components/ledger/internal/machine/vm/machine.go
- components/ledger/internal/machine/vm/program/program.go
Additional comments not posted (4)
components/ledger/internal/machine/vm/machine_test.go (4)
532-552: LGTM: New test case for unbounded overdraft source.This test case effectively verifies the behavior of sending from an account with unbounded overdraft. It ensures that the entire amount is taken from the unbounded source, which aligns with the expected behavior.
554-583: LGTM: Comprehensive test for multiple sources including unbounded overdraft.This test case effectively verifies the behavior of sending from multiple sources, including an account with unbounded overdraft. It ensures that the bounded account is used first, and then the remaining amount is taken from the unbounded source, which aligns with the expected behavior.
1024-1045: LGTM: New test case for balance tracking with mixed account types.This test case effectively verifies that an account's balance is tracked even if it appears later in the transaction as an unbounded overdraft source. This is important to ensure correct behavior when an account is used in multiple contexts within a single transaction.
Line range hint
1-2358: Overall assessment: Significant improvements to test coverage.The changes to this file greatly enhance the test coverage for the virtual machine implementation, particularly in handling unbounded overdrafts and various account types. The new test cases are well-structured and cover important scenarios. The introduction of the
mockStoretype provides a useful tool for simulating balance retrieval in tests.Key improvements:
- New test cases for unbounded overdraft sources
- Updated existing tests to handle different account types
- New tests for balance tracking in complex scenarios
- Introduction of a mock store for testing
These changes will help ensure the robustness and correctness of the virtual machine implementation.
ascandone
force-pushed
the
perf/do-no-lock-unbounded-sources
branch
from
8494a04
to
120e7e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (7)
components/ledger/internal/machine/script/compiler/compiler.go (2)
29-37: New fields added to parseVisitor structThe addition of
writeLockAccountsandreadLockAccountsfields to theparseVisitorstruct enhances the compiler's ability to track accounts that require locking during script execution. This change aligns with the PR objectives of optimizing the locking strategy for account balances.Consider documenting these new fields with more detailed comments, explaining their purpose and how they relate to the overall locking strategy. This will improve code maintainability and make it easier for other developers to understand the locking mechanism.
Line range hint
1-738: Overall assessment of changesThe modifications to the
compiler.gofile significantly enhance the compiler's ability to manage account locking, aligning well with the PR objectives of optimizing the locking strategy for account balances. The introduction ofwriteLockAccountsandreadLockAccountsfields in theparseVisitorstruct, along with the corresponding logic in theCompileFullfunction, provides a more granular approach to tracking accounts that require locking during script execution.These changes should lead to improved performance in handling unbounded overdraft sources, as the system can now differentiate between accounts that need write locks (bounded accounts in sources) and those that only require read locks (accounts in destinations or balance queries).
As the locking strategy becomes more complex, consider creating a separate package or module dedicated to managing account locking. This could include functions for determining lock types, managing lock conflicts, and optimizing lock acquisition and release. Such a modular approach would make it easier to maintain and potentially extend the locking mechanism in the future.
components/ledger/internal/machine/vm/machine_test.go (5)
554-583: LGTM: Comprehensive test for mixed source types.The
TestUnboundedSourceInorderfunction is a well-designed test case that verifies the behavior of sending from multiple sources, including an unbounded source account. It ensures that the system correctly prioritizes regular accounts before using the unbounded source.Consider adding a comment explaining the expected behavior:
func TestUnboundedSourceInorder(t *testing.T) { + // This test verifies that when sending from multiple sources, + // including an unbounded source, the system correctly prioritizes + // regular accounts before using the unbounded source. tc := NewTestCase() tc.compile(t, `send [GEM 15] ( source = {
Line range hint
975-1022: LGTM: Updated test case for balance locking.The changes to
TestNeededBalanceseffectively test the new functionality for handling different types of accounts, including world account, bounded overdrafts, and unbounded overdrafts. The use ofmockStoreis a good practice for isolating the test from actual data stores.Consider adding a brief comment explaining the purpose of each account type in the test script:
send [GEM 15] ( source = { + // Regular account, should be tracked $a @b + // World account, unbounded and should not be tracked max [GEM 1] from @world + // Bounded overdraft account, should be locked @bounded allowing overdraft up to [GEM 1] + // Unbounded overdraft account, should not be locked @unb allowing unbounded overdraft }
1024-1045: LGTM: New test case for mixed account usage.The
TestNeededBalances2function is a valuable addition that verifies the system's behavior when an account is used both as a regular source and an unbounded overdraft source within the same transaction. This ensures that the presence of an unbounded overdraft doesn't prevent necessary balance tracking.Consider adding a brief comment explaining the test's purpose:
func TestNeededBalances2(t *testing.T) { + // This test verifies that the system correctly tracks balances + // when an account is used both as a regular source and an + // unbounded overdraft source within the same transaction. p, err := compiler.Compile(` send [GEM 15] ( source = {
1047-1071: LGTM: New test case for balance function usage.The
TestNeededBalancesBalanceFnfunction is a valuable addition that verifies the system's behavior when using thebalance()function in the script. It ensures that the balance resolution process correctly identifies which accounts need to have their balances checked.Consider adding a brief comment explaining the test's purpose and expected behavior:
func TestNeededBalancesBalanceFn(t *testing.T) { + // This test verifies that when using the balance() function, + // the system correctly identifies which accounts need to have + // their balances checked. It should request balances for both + // the account used in balance() and the source account. p, err := compiler.Compile(`vars { monetary $balance = balance(@acc, COIN) }
2401-2417: LGTM: New mockStore for improved testing.The
mockStorestruct is a valuable addition for testing purposes. It allows for precise control over which accounts are requested during balance resolution, which is crucial for the new test cases. The implementation is simple and effective.Consider improving error handling in the
GetBalancemethod:func (s *mockStore) GetBalance(ctx context.Context, address, asset string) (*big.Int, error) { s.requestedAccounts = append(s.requestedAccounts, address) - return big.NewInt(0), nil + // Simulate a more realistic scenario + if address == "non_existent_account" { + return nil, fmt.Errorf("account not found") + } + return big.NewInt(100), nil // Return a non-zero balance for testing }Also, in the
GetAccountmethod, consider replacing thepanicwith a more graceful error return:func (s *mockStore) GetAccount(ctx context.Context, address string) (*ledger.Account, error) { - panic("not implemented") + return nil, fmt.Errorf("GetAccount: not implemented") }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- components/ledger/internal/engine/command/commander.go (2 hunks)
- components/ledger/internal/machine/script/compiler/compiler.go (4 hunks)
- components/ledger/internal/machine/script/compiler/compiler_test.go (2 hunks)
- components/ledger/internal/machine/script/compiler/destination.go (2 hunks)
- components/ledger/internal/machine/script/compiler/source.go (3 hunks)
- components/ledger/internal/machine/vm/machine.go (5 hunks)
- components/ledger/internal/machine/vm/machine_test.go (5 hunks)
- components/ledger/internal/machine/vm/program/program.go (1 hunks)
Files skipped from review due to trivial changes (1)
- components/ledger/internal/machine/script/compiler/compiler_test.go
Files skipped from review as they are similar to previous changes (5)
- components/ledger/internal/engine/command/commander.go
- components/ledger/internal/machine/script/compiler/destination.go
- components/ledger/internal/machine/script/compiler/source.go
- components/ledger/internal/machine/vm/machine.go
- components/ledger/internal/machine/vm/program/program.go
Additional comments not posted (4)
components/ledger/internal/machine/script/compiler/compiler.go (2)
687-694: Updates to CompileFull function initializationThe initialization of the
parseVisitorin theCompileFullfunction has been updated to include the newwriteLockAccountsandreadLockAccountsfields. This change ensures that these new tracking mechanisms are properly initialized.Let's verify if there are any other places in the codebase where
parseVisitoris initialized to ensure consistency:Verification successful
Initialization of
parseVisitoris consistent across the codebaseThe
parseVisitoris initialized only once in the codebase, and it includes thewriteLockAccountsandreadLockAccountsfields as expected. No other initializations were found, ensuring consistency.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of parseVisitor initialization rg --type go "parseVisitor\s*{" -C 10Length of output: 135
Script:
#!/bin/bash # Corrected search for other occurrences of parseVisitor initialization rg --type go "parseVisitor\s*\\{" -C 10Length of output: 1997
592-592: Update to VisitVars methodThe addition of
p.readLockAccounts[*accAddr] = struct{}{}in theVisitVarsmethod ensures that accounts referenced in balance queries are properly tracked for read locking. This change is consistent with the PR objectives and helps in optimizing the locking mechanism.To ensure this change doesn't introduce any unintended side effects, let's verify if there are any other places in the codebase where account balances are queried:
Verification successful
Change in VisitVars Method is Consistent and Safe
The addition of
p.readLockAccounts[*accAddr] = struct{}{}in theVisitVarsmethod aligns with the existing logic for managing read locks in the codebase. ThereadLockAccountsmap is used in various parts of the codebase to track accounts for read locking, and this change ensures that accounts involved in balance queries are properly tracked. There are no indications of unintended side effects from this change.
- The
readLockAccountsmap is used in conjunction with resource resolution and lock management in files likemachine.goandcommander.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of balance querying rg --type go "balance\(" -C 5Length of output: 20476
Script:
#!/bin/bash # Search for references to the VisitVars method in the codebase ast-grep --lang go --pattern 'func VisitVars($_) { $$$ }' # Search for references to the readLockAccounts map in the codebase rg --type go 'readLockAccounts'Length of output: 1967
components/ledger/internal/machine/vm/machine_test.go (2)
532-552: LGTM: New test case for unbounded source.The
TestUnboundedSourceSimplefunction is a well-structured test case that verifies the basic functionality of sending from an unbounded source account. It's a valuable addition to ensure the system correctly handles unbounded overdrafts.
Line range hint
1-2417: Overall improvements to test coverage and balance tracking.The changes in this file significantly enhance the test coverage for handling unbounded overdrafts and improve balance tracking. Key improvements include:
- New test cases for unbounded sources (
TestUnboundedSourceSimpleandTestUnboundedSourceInorder).- Updates to
TestNeededBalancesto cover various account types and overdraft scenarios.- New test cases
TestNeededBalances2andTestNeededBalancesBalanceFnto verify balance tracking in complex scenarios.- Introduction of the
mockStorestruct for more precise testing of balance resolution.These changes collectively strengthen the test suite and ensure that the system correctly handles various account types and balance tracking scenarios. The use of
mockStoreis particularly valuable for isolating tests and verifying balance resolution behavior.
| readLockAccounts := make(machine.Addresses, 0) | ||
| for address := range visitor.readLockAccounts { | ||
| readLockAccounts = append(readLockAccounts, address) | ||
| } | ||
| sort.Stable(readLockAccounts) | ||
|
|
||
| writeLockAccounts := make(machine.Addresses, 0) | ||
| for address := range visitor.writeLockAccounts { | ||
| writeLockAccounts = append(writeLockAccounts, address) | ||
| } | ||
| sort.Stable(sources) | ||
| sort.Stable(writeLockAccounts) | ||
|
|
||
| artifacts.Program = &program.Program{ | ||
| Instructions: visitor.instructions, | ||
| Resources: visitor.resources, | ||
| NeededBalances: visitor.neededBalances, | ||
| Sources: sources, | ||
| Instructions: visitor.instructions, | ||
| Resources: visitor.resources, | ||
| NeededBalances: visitor.neededBalances, | ||
| ReadLockAccounts: readLockAccounts, | ||
| WriteLockAccounts: writeLockAccounts, |
There was a problem hiding this comment.
New logic for handling ReadLockAccounts and WriteLockAccounts
The addition of logic to process readLockAccounts and writeLockAccounts in the CompileFull function ensures that these accounts are properly sorted and included in the final Program structure. This change is crucial for implementing the new locking strategy.
Consider extracting the logic for processing readLockAccounts and writeLockAccounts into separate helper functions to improve code readability and maintainability. For example:
func processLockAccounts(accounts map[machine.Address]struct{}) machine.Addresses {
result := make(machine.Addresses, 0, len(accounts))
for address := range accounts {
result = append(result, address)
}
sort.Stable(result)
return result
}
// Usage in CompileFull:
readLockAccounts := processLockAccounts(visitor.readLockAccounts)
writeLockAccounts := processLockAccounts(visitor.writeLockAccounts)This refactoring would reduce code duplication and make the CompileFull function more concise.
ascandone
force-pushed
the
perf/do-no-lock-unbounded-sources
branch
from
120e7e3
to
9e71571
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (4)
components/ledger/internal/machine/vm/machine_test.go (4)
554-583: LGTM: Comprehensive test for mixed source types.The
TestUnboundedSourceInorderfunction is a well-designed test case that verifies the behavior of sending from multiple sources, including an unbounded source account. It ensures that the system correctly prioritizes regular accounts before using the unbounded source.Consider adding a comment explaining the expected behavior:
// This test ensures that when sending from multiple sources, // including an unbounded source, the system prioritizes // regular accounts before using the unbounded source.
2342-2363: LGTM: Test case for minimal unbounded repayment scenario.The
TestRepayUnboundedMinimalfunction is a well-structured test case that verifies the behavior of sending from an unbounded source to multiple destinations with max limits. It ensures that the system correctly handles this use case.Consider adding a brief comment explaining the purpose of this test:
// This test verifies that when sending from an unbounded source // to multiple destinations with max limits, the system correctly // allocates the amounts according to the specified limits.
2365-2399: LGTM: Comprehensive test for complex unbounded repayment scenario.The
TestRepayUnboundedComplexfunction is an excellent test case that verifies the behavior of sending from an unbounded source to multiple destinations with various max limits. It ensures that the system correctly handles intricate allocation scenarios.Consider adding a brief comment explaining the purpose and complexity of this test:
// This test verifies a complex scenario where funds are sent from an unbounded source // to multiple destinations with various max limits. It ensures that the system // correctly allocates amounts according to the specified limits and priorities.
Line range hint
981-1021: LGTM: Improved test coverage for different account types.The changes to the
TestNeededBalancesfunction enhance the test coverage by including cases for world account, bounded overdraft, and unbounded overdraft. This is a valuable update to ensure the system correctly handles different account types in terms of balance tracking and locking.Consider adding a brief comment explaining the purpose of each test case:
// Test cases: // 1. Regular accounts (should be tracked) // 2. World account (should not be tracked as it's unbounded) // 3. Bounded overdraft account (should be locked) // 4. Unbounded overdraft account (should not be locked)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- components/ledger/internal/machine/vm/machine.go (5 hunks)
- components/ledger/internal/machine/vm/machine_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/ledger/internal/machine/vm/machine.go
Additional comments not posted (2)
components/ledger/internal/machine/vm/machine_test.go (2)
532-552: LGTM: New test case for unbounded source.The
TestUnboundedSourceSimplefunction is a well-structured test case that verifies the basic functionality of sending from an unbounded source account. It's a valuable addition to ensure the system correctly handles unbounded overdrafts.
Line range hint
532-2417: Excellent improvements to test coverage.The changes in this file significantly enhance the test suite by adding several new test cases and a
mockStorestruct for testing purposes. These additions cover important scenarios such as:
- Simple and complex unbounded source transactions
- Mixed source types (bounded and unbounded)
- Complex repayment scenarios with multiple destinations and limits
- Handling of different account types (world, bounded overdraft, unbounded overdraft)
These improvements will help ensure the robustness and correctness of the system when dealing with various transaction scenarios, particularly those involving unbounded sources and complex allocations.
| type mockStore struct { | ||
| requestedAccounts []string | ||
| } | ||
|
|
||
| func (s *mockStore) GetRequestedAccounts() []string { | ||
| slices.Sort(s.requestedAccounts) | ||
| return s.requestedAccounts | ||
| } | ||
|
|
||
| func (s *mockStore) GetBalance(ctx context.Context, address, asset string) (*big.Int, error) { | ||
| s.requestedAccounts = append(s.requestedAccounts, address) | ||
| return big.NewInt(0), nil | ||
| } | ||
|
|
||
| func (s *mockStore) GetAccount(ctx context.Context, address string) (*ledger.Account, error) { | ||
| panic("not implemented") | ||
| } |
There was a problem hiding this comment.
LGTM: New mockStore type for testing, with suggested improvements.
The mockStore struct and its methods are well-implemented for testing purposes. They provide the necessary functionality for tracking requested accounts and simulating balance retrieval.
Consider the following improvements:
- Implement the
GetAccountmethod to return an error instead of panicking:
func (s *mockStore) GetAccount(ctx context.Context, address string) (*ledger.Account, error) {
return nil, fmt.Errorf("GetAccount: not implemented")
}- Add a comment explaining the purpose of the
mockStore:
// mockStore is a test implementation of the Store interface
// used for tracking requested accounts and simulating balance retrieval.
type mockStore struct {
requestedAccounts []string
}- Consider adding a mutex to make the
mockStoresafe for concurrent use:
type mockStore struct {
requestedAccounts []string
mu sync.Mutex
}
func (s *mockStore) GetBalance(ctx context.Context, address, asset string) (*big.Int, error) {
s.mu.Lock()
defer s.mu.Unlock()
s.requestedAccounts = append(s.requestedAccounts, address)
return big.NewInt(0), nil
}
Linear ticket: https://linear.app/formance/issue/ENG-1303/avoid-tracking-accounts-balances-when-not-necessary
TL;DR: we are trying to avoid locking balances for unbounded sources, in order to improve perf.
Locking strategy before the PR:
readlocks were requested for each account that appeared in the program (excluding @world)
writelocks were requested for each account that appeared in the sources (excluding @world)
Locking strategy after the PR:
writelocks are requested for each bounded account (that is: accounts that aren't world or aren't unbounded overdrafts) that appears in sources.
readlocks are requested for each account that appears in destinations or in the
balance()function. NOTE: this means that unlike the current implementation, readlocks do not include accounts that already have a write lockChanges in emitted bytecode:
@worldand@acc allowing unbounded overdraftsources now emit theTAKE_ALWAYSopcode instead of emitting theTAKE_ALLopcode. This allows them to send the required amount without checking if the balance is enough (which, in turn, allows the machine to avoid locking those accounts' balances)Changes in bytecode semantics
the
OP_TAKE_ALWAYSnow allows sending from an account even if it's balance is not found (this is the case with@worldor with unbounded accounts)