Update settlement contract balances in background task #2108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MartinquaXD
reviewed
Dec 1, 2023
MartinquaXD
approved these changes
Dec 1, 2023
squadgazzz
approved these changes
Dec 1, 2023
| if let Some(balance) = balances.remove(key) { | ||
| entry.balance = balance; | ||
| } else { | ||
| tracing::info!(?key, "key without balance update"); |
There was a problem hiding this comment.
What is the purpose of the INFO level here?
There was a problem hiding this comment.
It's supposed to help us to debug the rare race condition where we add a new token balance to the cache while the cache update is in flight.
The new item may not be updated (it should be up to date but in theory the block could also have just changed) and therefore we could pass a slightly outdated buffer list to the solvers. Again, I think this is extremely rare, but if it happens we can at least see if from the logs now.
MartinquaXD
reviewed
Dec 1, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Addresses the bug that we currently fetch settlement buffer balances once per token and don't update them in between auctions, which can lead to solutions relying on outdate internal buffer balance information and thus incorrectly marking interactions as internalizable causing simulation to fail.
Changes
This approach is a bit wasteful, as token balances only really change when a settlement trading this token is included on-chain. However, getting that information in this component is non-trivial. Moreover, some tokens exhibit custom balance logic (e.g. rebasing tokens) which could update the contract balance even without a settlement taking place.
If we deem this approach too inefficient, I would suggest we restrict the number of tokens we cache (using an LRU cache) and re-fetch all relevant information (including decimals and symbol) in case of a cache miss.
If we don't want to lose caching of decimals and symbols for some tokens, I'd suggest splitting the fetcher into two components, one for static information, and one more
RecentBlockCachelike component for the settlement balances.Let me know what you think.
How to test
Added an end to end test which fails without the changes in
driver::infra::tokensRelated Issues
Fixes #2093