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.

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

Fix caching eth calls #731

Merged
merged 4 commits into from
Jan 6, 2025
Merged

Fix caching eth calls #731

merged 4 commits into from
Jan 6, 2025

Conversation

gndelia
Copy link
Contributor

@gndelia gndelia commented Jan 4, 2025

Description

It's been a while since I've noticed that the caching wasn't properly working for the RPC calls for withdrawals. I took some time to debug and found the bug. This PR fixes this, so the eth_call methods that we defined some time ago as cacheable can be cached again.

Context

The process of syncing deposits and withdrawals requires lots of requests. Many of them can be cached at block level, and some of them can be cached in memory as long as the user doesn't close the tab.

Many of the requests are run in the form of an eth_call, so the request body looks like this

{
  "method": "eth_call",
  "params": [
    { "to": "0xb6f9579980ae46f61217a99145645341e49e2516", "data": "0x54fd4d50" },
    "latest",
  ],
  "id": 320,
  "jsonrpc": "2.0",
}

That data field maps to calling version() in the bridge contract (The data field is the hash that comes from doing keccak256(toHex('version()')).slice(0, 10))

Some time ago, I defined (with Gabriel's help) some methods to be cached - the list can be found here.

There's an in-memory object that indexes a strategy for each method that states how long a response should be stored (Basically, per-block basis or permanently)

The bug

The cache was not working

Root cause

In order to cache only those methods in the list, there was this method

const isWithdrawalEthCall = function (obj: unknown): obj is RpcCallParam {
  const casted = obj as RpcCallParam
  return casted !== undefined && isHash(casted.data)

which verifies the eth_call is indeed an eth_call. However, this check had a problem, which becomes evident by looking at the viem's source code of isHash() function:~~

function isHash(hash: string): hash is Hex {
  return isHex(hash) && size(hash) === 32
}

The function returns true if the string is a hex string and if size (do not confuse with length) is 32...

This is valid for Transaction hashes, but not for the hashes of eth_call !! As these are hashes (in the sense of computational hashes) but not transaction hashes from the blockchain.. (you can check the body of the eth_call again above)

This also means that the caching never worked properly 🤦🏽

The solution

The correct check was to verify that the string was a hex using isHex(data), instead of using isHash(data)

As the library eth-rpc-cache ensures only eth_call are forwarded to the strategy, and all eth_call have the same structure (with the data field), we can remove the function isWithdrawalEthCall altogether, so the buggy code doesn't execute anymore


  • 6f43064 This commit applies the fix
  • 78bf7a2 moves the function cacheProvider outside of ui-common, again, in another effort of completely removing ui-common package (As this is only used by the Portal)
  • 64dc364 adds a few tests for the withdrawal strategy.

Screenshots

No changes are visible to the user... though now many requests are cached and will be skipped 🎉

Checklist

  • Manual testing passed.
  • Automated tests added, or N/A.
  • Documentation updated, or N/A.
  • Environment variables set in CI, or N/A.

@gndelia gndelia self-assigned this Jan 4, 2025
@gndelia gndelia requested a review from gabmontes as a code owner January 4, 2025 22:58
ArturDolzan
ArturDolzan previously approved these changes Jan 6, 2025
Copy link
Contributor

@ArturDolzan ArturDolzan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uACK

Copy link
Contributor

@gabmontes gabmontes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we even need to call isWithdrawalEthCall within the resolver. If the calls are made through an SDK/library, those will have the proper structure.

@gndelia
Copy link
Contributor Author

gndelia commented Jan 6, 2025

I don't think we even need to call isWithdrawalEthCall within the resolver. If the calls are made through an SDK/library, those will have the proper structure.

It is a minor safeguard to avoid trying to .slice over the data field if it is not an eth_call. I agree it may be a bit over-defensive. Would you prefer me to remove it @gabmontes ?

@gndelia gndelia force-pushed the fix-caching-eth-calls branch from 65089b8 to fa1de87 Compare January 6, 2025 19:44
@gndelia
Copy link
Contributor Author

gndelia commented Jan 6, 2025

After discussing online with @gabmontes , I proceed to remove the isWithdrawalEthCall check in fa1de87, as the library eth-rpc-cache ensures only eth_call are forwarded to this strategy

Copy link
Contributor

@gabmontes gabmontes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gndelia gndelia merged commit 226941c into main Jan 6, 2025
6 checks passed
@gndelia gndelia deleted the fix-caching-eth-calls branch January 6, 2025 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants