Bound native price cache with moka

[squadgazzz]

Description

The native price cache is backed by an unbounded Mutex<HashMap<Address, CachedResult>>. Entries are inserted on every price estimate but never removed. Once at steady state with many unique tokens, the cache grows without bound.

The size limit is currently hardcoded at 20k, since on mainnet we have around that number of unique tokens:

SELECT COUNT(*) AS unique_token_count
FROM (SELECT sell_token AS token
      FROM orders
      UNION
      SELECT buy_token AS token
      FROM orders) t;

Returns -> 20109

Per cache entry breakdown:

• CachedResult value: ~32 bytes
• Address key: 20 bytes
• Moka internal overhead: ~100-120 bytes
• Total per entry: ~170 bytes

At 20,000 entries: ~3.4 MB upper bound

Changes

• Replace the Mutex<HashMap<...>> cache with moka::sync::Cache.
• Adapt get_cached_price():
  • No more MutexGuard parameter. Use data.get(&token) which returns Option (moka clones the value). Check staleness + is_ready() on the returned value. No requested_at to update, since moka tracks access internally.
• Remove the requested_at field from the CachedResult since it is not needed anymore and turned out to be dead code.
• Adapt the insert() function to use entry_by_ref().and_compute_with(), which gives gives atomic read-modify-write for accumulative_errors_count.

How to test

Existing tests.

[squadgazzz]

/gemini review

[gemini-code-assist]

Code Review

This pull request replaces an unbounded HashMap with a bounded moka cache to mitigate a potential memory exhaustion vulnerability. The implementation is largely correct, but includes a potential integer overflow on 32-bit systems when casting the cache entry count. My review includes a suggestion to address this.

[gemini-code-assist]

Code Review

This pull request replaces an unbounded Mutex<HashMap> with a bounded moka::sync::Cache for the native price cache to address a potential memory exhaustion vulnerability. The implementation adapts the existing logic to moka's API. The review identified one high-severity issue: a potential panic on 32-bit systems due to an integer conversion in the len() method, which could lead to a denial of service.

[jmg-duarte]

The PR and commit don't agree on the type of cache

This is also a bit confusing:

Per entry: CachedResult (~32 bytes value) + Address key (20 bytes) + moka overhead (~100-120 bytes for internal tracking) ≈ ~170 bytes. 20,000 × 170 ≈ 3.4 MB.

Separating the total would make the description much easier to understand

[squadgazzz]

The PR and commit don't agree on the type of cache

It doesn't matter, the squashed commit will have the PR's title message. Renamed the PR a bit. And updated the breakdown description.

[jmg-duarte]

It doesn't matter, the squashed commit will have the PR's title message.
In the end it doesn't yes, but for me as a reviewer it was confusing. Thanks for updating

[jmg-duarte]

You can add the napkin math you did here to help understand why 20k

[squadgazzz]

Git blame should work better for that to avoid long comments.

[jmg-duarte]

Git blame helps but this is what comments are ultimately for. The issue with git blame is that as the file gets moved around one will need to perform an archeological excavation to find out the origin — it's not impossible, just time consuming

Regardless, in this case, it's not that important

[m-sz]

LGTM, small nits.