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

test: fix hive tests re: genesis smoke test #1530

Merged
merged 16 commits into from
Mar 1, 2023

Conversation

fkrause98
Copy link
Contributor

@fkrause98 fkrause98 commented Feb 23, 2023

Fixes #1489

Modifies the storage encoding to exit early on an empty storage map, and use U256 instead of H256 for encoding individual values.

@gakonst
Copy link
Member

gakonst commented Feb 24, 2023

@MegaRedHand @fkrause98 can you please allow members of the repo to be pushing to your PRs?^^

@onbjerg onbjerg changed the title feat: Hive tests fix test: fix hive test Feb 24, 2023
@onbjerg onbjerg changed the title test: fix hive test test: fix hive tests Feb 24, 2023
@onbjerg onbjerg added the C-test A change that impacts how or what we test label Feb 24, 2023
@fkrause98
Copy link
Contributor Author

@gakonst
Done, please let us know if anyone else needs push permission.

@gakonst
Copy link
Member

gakonst commented Feb 24, 2023

@Rjected can you please take this over so that we can get the hive genesis smoke test passing?

@fkrause98 As a process point: this is part of the blocker PRs for Hive, so if you're opening it as a Draft, ideally we'd like to figure out a way to move it forward. Here it seems like it's NBD as it's just a small bug, but if you guys take on blocker issues let's try to have some more active coordination on how to not let them stagnate.

@Rjected
Copy link
Member

Rjected commented Feb 24, 2023

Sounds good, I'll take this one over the line.

@fkrause98
Copy link
Contributor Author

fkrause98 commented Feb 27, 2023

Ok!

And @Rjected, I've been debugging this for a bit, I've found out that the state root for this config is being returned as 0xbdf2d9a7f05e669cacafa199298ab1ee68ec2bb457c516897f22c5971b434591 whereas geth returns 0x9a6049ac535e3dc7436c189eaa81c73f35abd7f282ab67c32944ff0301d63360.

Leaving this for you then.

@Rjected
Copy link
Member

Rjected commented Feb 27, 2023

thanks! I've figured out that we are not parsing the json correctly, the alloc storage fields are missing from the chainspec, which explains the incorrect state root

@Rjected
Copy link
Member

Rjected commented Feb 27, 2023

fixed parsing here:
gakonst/ethers-rs#2205

@Rjected
Copy link
Member

Rjected commented Feb 27, 2023

working on another issue with computing the state root after fixing parsing, so there were bugs in both areas!

@Rjected Rjected force-pushed the fix_smoke_hive_tests branch 3 times, most recently from 1dd1691 to 79df922 Compare February 28, 2023 03:45
@Rjected Rjected marked this pull request as ready for review February 28, 2023 03:46
@gakonst
Copy link
Member

gakonst commented Feb 28, 2023

ethers-rs#2205 is merged @Rjected

@Rjected
Copy link
Member

Rjected commented Feb 28, 2023

@gakonst rebased and updated ethers

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2023

Codecov Report

Merging #1530 (05ad5b2) into main (dc2f604) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1530      +/-   ##
==========================================
- Coverage   75.09%   75.09%   -0.01%     
==========================================
  Files         372      372              
  Lines       43613    43695      +82     
==========================================
+ Hits        32752    32813      +61     
- Misses      10861    10882      +21     
Flag Coverage Δ
integration-tests 21.39% <2.27%> (-0.05%) ⬇️
unit-tests 69.66% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crates/primitives/src/chain/spec.rs 97.87% <100.00%> (+0.87%) ⬆️
crates/primitives/src/genesis.rs 90.00% <100.00%> (+6.66%) ⬆️
crates/transaction-pool/src/test_utils/mock.rs 61.64% <0.00%> (-6.51%) ⬇️
crates/net/eth-wire/src/capability.rs 74.26% <0.00%> (-0.74%) ⬇️
crates/net/network/src/session/active.rs 84.59% <0.00%> (-0.60%) ⬇️
crates/transaction-pool/src/pool/txpool.rs 61.06% <0.00%> (-0.50%) ⬇️
crates/net/discv4/src/lib.rs 65.67% <0.00%> (-0.15%) ⬇️
crates/net/network/src/peers/manager.rs 82.46% <0.00%> (-0.09%) ⬇️
crates/net/eth-wire/src/ethstream.rs 84.44% <0.00%> (+0.47%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

fkrause98 and others added 8 commits February 28, 2023 13:15
Co-authored-by: lambdaclass-user <github@lambdaclass.com>
Co-authored-by: lambdaclass-user <github@lambdaclass.com>
Co-authored-by: lambdaclass-user <github@lambdaclass.com>
Co-authored-by: lambdaclass-user <github@lambdaclass.com>
Co-authored-by: lambdaclass-user <github@lambdaclass.com>
Co-authored-by: lambdaclass-user <github@lambdaclass.com>
@onbjerg
Copy link
Member

onbjerg commented Feb 28, 2023

Does this close #1489? If so, please mark it as such by typing "Closes #1489" or similar in the PR description

@Rjected
Copy link
Member

Rjected commented Feb 28, 2023

Does this close #1489? If so, please mark it as such by typing "Closes #1489" or similar in the PR description

Yep, just added

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

awesome - thank you

@gakonst gakonst merged commit 4839ade into paradigmxyz:main Mar 1, 2023
@gakonst gakonst changed the title test: fix hive tests test: fix hive tests re: genesis smoke test Mar 1, 2023
@MegaRedHand MegaRedHand deleted the fix_smoke_hive_tests branch March 1, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-test A change that impacts how or what we test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect hashes in genesis smoke test
5 participants