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: block value in produceBlockV3 should be in wei #6286

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

ensi321
Copy link
Contributor

@ensi321 ensi321 commented Jan 12, 2024

Description

Currently consensusBlockValue is in Gwei. To align with executionPayloadValue, block value is converted into Wei.

Note that values in RewardCache will still be in Gwei for the purpose of rewards API, which is denominated in Gwei.

@ensi321 ensi321 marked this pull request as ready for review January 12, 2024 04:31
@ensi321 ensi321 requested a review from a team as a code owner January 12, 2024 04:31
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Merging #6286 (3c20598) into unstable (aa87c54) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #6286      +/-   ##
============================================
- Coverage     80.38%   80.38%   -0.01%     
============================================
  Files           202      202              
  Lines         19620    19619       -1     
  Branches       1176     1176              
============================================
- Hits          15771    15770       -1     
  Misses         3821     3821              
  Partials         28       28              

@nflaig
Copy link
Member

nflaig commented Jan 12, 2024

Note that values in RewardCache will still be in Gwei for the purpose of rewards API, which is denominated in Gwei.

You are right the spec states to use gwei for rewards API but isn't this a bit odd? I feel like we should use the same unit across all APIs

@ensi321
Copy link
Contributor Author

ensi321 commented Jan 12, 2024

Note that values in RewardCache will still be in Gwei for the purpose of rewards API, which is denominated in Gwei.

You are right the spec states to use gwei for rewards API but isn't this a bit odd? I feel like we should use the same unit across all APIs

Yea I have pointed this out in api channel on Discord.

I think we either be inconsistent with payload value in the same endpoint, or be inconsistent with reward value from other endpoints. It's unideal either way.

@ensi321
Copy link
Contributor Author

ensi321 commented Jan 12, 2024

I just realized the beacon API PR that proposes block value in Wei hasn't been merged yet. We can hold off merging this until that PR is merged.

@nflaig
Copy link
Member

nflaig commented Jan 12, 2024

We can hold off merging this until that PR is merged.

yeah, let's wait a bit until there is a final decision on this and the spec PR is merged

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

blocked till the spec is updated

@g11tech
Copy link
Contributor

g11tech commented Jan 12, 2024

@ensi321 can you also link the spec PR in the description (it may be in the linked issue but direct linking here is more useful)

@ensi321
Copy link
Contributor Author

ensi321 commented Jan 15, 2024

yeah, let's wait a bit until there is a final decision on this and the spec PR is merged

PR is merged. I believe we can continue reviewing this PR @nflaig @g11tech

Copy link
Member

@nflaig nflaig left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

lgtm

@wemeetagain wemeetagain dismissed g11tech’s stale review January 15, 2024 20:47

spec PR now merged

@wemeetagain wemeetagain merged commit 4ec413c into ChainSafe:unstable Jan 16, 2024
16 of 18 checks passed
ensi321 added a commit to ensi321/lodestar that referenced this pull request Jan 22, 2024
@wemeetagain
Copy link
Member

🎉 This PR is included in v1.15.0 🎉

jeluard pushed a commit to jeluard/lodestar that referenced this pull request Jan 30, 2024
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.

Payload and block values should be in wei
4 participants