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

ncli verifyDeposit: a simple helper for inspecting deposit events #3855

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

zah
Copy link
Contributor

@zah zah commented Jul 11, 2022

No description provided.

@tersec
Copy link
Contributor

tersec commented Jul 11, 2022

This nim-web3 bump isn't safe: #3850

That's an issue in nim-web3, and I'll revert the changes there. This Quantity thing is a real spec divergence, but also, shouldn't block other things in the meantime.

Also, because of the other spec ambiguities detailed there, already reported, it's not clear exactly what "correct" handling of the "Quantity" difficulty/totalDifficulty would be, so it can't be made unambiguously correct, pending JSON-RPC clarifications.

None of which is directly relevant here, just, why the fix isn't just-add-one-more-change-to-ethtypes instead of revert.

@tersec
Copy link
Contributor

tersec commented Jul 11, 2022

It shouldn't need a full revert -- there's one specific condition which causes problems currently due to the somewhat less-than-rigorous Quantity usage (e.g., for various hashes which are definitely not defined as integer quantities, too): status-im/nim-web3#55

@github-actions
Copy link

Unit Test Results

       12 files  ±0       857 suites  ±0   1h 18m 28s ⏱️ + 14m 15s
  1 904 tests ±0    1 756 ✔️ ±0  148 💤 ±0  0 ±0 
10 328 runs  ±0  10 122 ✔️ ±0  206 💤 ±0  0 ±0 

Results for commit dcaa8c7. ± Comparison against base commit 571577c.

eth2_ssz_serialization, forks, helpers, state_transition, signatures],
../beacon_chain/networking/network_metadata,
../beacon_chain/conf/eth2_types_confutils_defs,
../beacon_chain/eth1/deposit_contract_abi
Copy link
Member

Choose a reason for hiding this comment

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

this import is quite unfortunate: it brings in the entirerty of web3 which transitively imports lots and lots of completely irrelevant stuff just to get access to a trivial byte converter

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