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

Fixes getStorageAt undeployed contract response #2250

Merged
merged 4 commits into from
Nov 6, 2024
Merged

Conversation

weiihann
Copy link
Contributor

@weiihann weiihann commented Nov 4, 2024

Trie.Get by default returns no error and zero value for non-existent key. To check if a contract is deployed, we have no choice by to check if the nonce also exists (which is a redundant DB IO).

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.35%. Comparing base (e4240ac) to head (9e72cf1).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
rpc/contract.go 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2250      +/-   ##
==========================================
+ Coverage   75.25%   75.35%   +0.09%     
==========================================
  Files         106      106              
  Lines       11229    11238       +9     
==========================================
+ Hits         8450     8468      +18     
+ Misses       2138     2133       -5     
+ Partials      641      637       -4     

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

core/trie/trie.go Outdated Show resolved Hide resolved
fix

Revert "Fixes getStorageAt undeployed contract response"

This reverts commit bd44ef2d79ace6abe9358c053ea3b640dc389bb0.

fix

fix test

fix

fix
Copy link
Contributor

@IronGauntlets IronGauntlets left a comment

Choose a reason for hiding this comment

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

It seems to me that the problem you are describing would be better solved by returning an error from stateReader.ContractStorage when the key doesn't exist, right?

rpc/contract.go Show resolved Hide resolved
@weiihann
Copy link
Contributor Author

weiihann commented Nov 5, 2024

It seems to me that the problem you are describing would be better solved by returning an error from stateReader.ContractStorage when the key doesn't exist, right?

If the key doesn't exist, it will return a zero value for ContractStorage (see Trie.Get). This behavior of Trie is correct because a non-existent key by default has a zero value, so we shouldn't modify it to return the key not found error.

The problem is that we need a way to check if a contract exists. Since we put contract fields (i.e. nonce, balance, storage) in different buckets, we have no choice but to add an additional DB lookup (in this case using ContractNonce) to check if a contract is deployed or not.

If we condensed the contract fields into a single bucket (see #2224), we only need to do a single DB lookup to check if a contract exists or not.

rpc/contract.go Outdated Show resolved Hide resolved
@kirugan kirugan merged commit 5605db6 into main Nov 6, 2024
13 checks passed
@kirugan kirugan deleted the weiihann/fix-storageat branch November 6, 2024 08: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