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(katana-provider): temporarily invalidate cache if nonce or class hash is default value #1850

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

kariy
Copy link
Member

@kariy kariy commented Apr 18, 2024

invalidate the local nonce or class hash value for forked provider if they are 0.

The reason why we do this is because the nonce and class hash are stored within the same struct (ie GenericContractInfo), and to give some context, forked provider works like this:

  1. find value in local storage
  2. if value exist, return. but if not,
  3. fetch from remote network, and store in local storage

these steps are done for each of the methods within the StateProvider trait. So when the StateProvider::nonce() is called, it will find in local, if not found then fallback to remote network. Once it receives the value from the remote network, it will store locally in the GenericContractInfo struct. AND BECAUSE this struct is used to store both the nonce AND the class hash, so when we create it for the first time (either thru StateProvider::nonce() or StateProvider::class_hash_of_contract()), the other field would be set to its default value.

The issue happens when you're calling StateProvider::class_hash_of_contract() after StateProvider::nonce() (or vice versa), the class hash returned will be ZERO. Because it sees that the corresponding GenericContractInfo exist and extract the class hash from there.


NOTE:

this would mainly be a temporary fix until we're able to use a better data types to represent 'not yet fetched' and 'default value'.

Copy link

codecov bot commented Apr 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.53%. Comparing base (9c47ba7) to head (2bf8c37).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1850      +/-   ##
==========================================
+ Coverage   70.51%   70.53%   +0.01%     
==========================================
  Files         309      309              
  Lines       35024    35041      +17     
==========================================
+ Hits        24698    24716      +18     
+ Misses      10326    10325       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

thanks @kariy for the temporary fix. Will also add tests with the Katana runner in an other PR.

@glihm
Copy link
Collaborator

glihm commented Apr 18, 2024

Hum, doing more tests, I've having some issues on subsequent transactions.
The first one works fine, but then I have nonce error and contract not deployed again.
Investigating.

Copy link
Collaborator

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Thanks for the latest fix, this one is doing the job. 👍

@kariy kariy merged commit dbad879 into dojoengine:main Apr 19, 2024
12 checks passed
@kariy kariy deleted the fix-forked branch April 19, 2024 00:20
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.

2 participants