Skip to content

Fixes getStorageAt undeployed contract response#2250

Merged
kirugan merged 4 commits intomainfrom
weiihann/fix-storageat
Nov 6, 2024
Merged

Fixes getStorageAt undeployed contract response#2250
kirugan merged 4 commits intomainfrom
weiihann/fix-storageat

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).

@weiihann weiihann requested a review from kirugan November 4, 2024 13:54
@weiihann weiihann force-pushed the weiihann/fix-storageat branch from a070ef8 to ab431d2 Compare November 4, 2024 13:54
@codecov
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.

@weiihann weiihann force-pushed the weiihann/fix-storageat branch from ab431d2 to 47b8084 Compare November 4, 2024 15:06
fix

Revert "Fixes getStorageAt undeployed contract response"

This reverts commit bd44ef2d79ace6abe9358c053ea3b640dc389bb0.

fix

fix test

fix

fix
@weiihann weiihann force-pushed the weiihann/fix-storageat branch from 47b8084 to 3e3564b Compare November 4, 2024 15:27
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?

@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.

@kirugan kirugan merged commit 5605db6 into main Nov 6, 2024
@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