Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bug: Settlement contract balances get cached too long #2093

Closed
MartinquaXD opened this issue Nov 28, 2023 · 2 comments · Fixed by #2108
Closed

bug: Settlement contract balances get cached too long #2093

MartinquaXD opened this issue Nov 28, 2023 · 2 comments · Fixed by #2108
Assignees
@fleupold
Labels
bug Something isn't working E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details

Comments

@MartinquaXD
Copy link
Contributor

Problem

For some solvers the driver has to provide some metadata (symbol, decimals, settlement contract balance).
This is already cached (nice) but at least the settlement contract balance should be updated on every new block.

Impact

We are reporting outdated token balances which can affect internal buffer trading optimizations.

Expected behaviour

The balance of every token in the balance should be the value at the current block when building the auction in the driver.
@MartinquaXD MartinquaXD added bug Something isn't working E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details labels Nov 28, 2023
@fleupold
Copy link
Contributor

Thinking about this again, this may actually be blocking launch since it would lead to wrong or significantly less internalisations on the solver side (which has a big impact on gas costs on mainnet production)

@fleupold
Copy link
Contributor

My suggestion would be to use CurrentBlockStream and update a fixed amount of recently used and invalidate remaining balances (so that they get fetched when queried) whenever a new block arrives.

@fleupold fleupold self-assigned this Dec 1, 2023
@squadgazzz squadgazzz assigned squadgazzz and unassigned fleupold Dec 1, 2023
@fleupold fleupold mentioned this issue Dec 1, 2023
Merged
2 tasks
@squadgazzz squadgazzz removed their assignment Dec 1, 2023
@fleupold fleupold self-assigned this Dec 4, 2023
fleupold added a commit that referenced this issue Dec 4, 2023
@fleupold
Update settlement contract balances in background task (#2108)
39f4f4a 
# 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
- [ ] Add an e2e showcasing the problematic behavior
- [ ] Spawn a task when creating a new token fetcher, which on arrival
of new blocks update the token balances of all cached tokens.

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 `RecentBlockCache` like 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::tokens`

## Related Issues

Fixes #2093
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fleupold fleupold

Labels
bug Something isn't working E:3.1 Driver Colocation See https://github.com/cowprotocol/pm/issues/14 for details
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update settlement contract balances in background task cowprotocol/services
3 participants
@fleupold @MartinquaXD @squadgazzz