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

Return payload value in Wei from /eth/v3/validator/blocks #13497

Merged
merged 10 commits into from
Jan 24, 2024

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Jan 22, 2024

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

  • Fixes a bug in consensus-types/blocks/getters.go where calling Execution() would return a new wrapped object with 0 value. Actually there is no need to unwrap and re-wrap, we can just return the field value directly. Making sure it's of the correct type should be done in the setter.
  • Adds a ValueInWei() method to the ExecutionData interface so that we can retrieve the payload value both in Wei and Gwei. The /eth/v3/validator/blocks API endpoint should return Wei values.

Which issues(s) does this PR fix?

Fixes #13466
Aligns with ethereum/beacon-APIs#400

Other notes for review

@rkapka rkapka added Ready For Review A pull request ready for code review API Api related tasks labels Jan 22, 2024
@rkapka rkapka requested a review from a team as a code owner January 22, 2024 17:13
@rkapka rkapka force-pushed the produce-block-wei branch from 5d939cd to 071a1a1 Compare January 22, 2024 17:13
@@ -435,6 +435,9 @@ func (s *Service) validateMergeTransitionBlock(ctx context.Context, stateVersion
if err != nil {
return invalidBlock{error: err}
}
if payload == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this should exist... doesn't the check below is empty execution data deal with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, I moved this check to IsEmptyExecutionData

value uint64
p *enginev1.ExecutionPayloadCapella
weiValue math.Wei
gweiValue uint64
Copy link
Member

Choose a reason for hiding this comment

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

do we still need to keep gweiValue now we have weiValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept gweiValue so that we don't have to convert every time we want to get the value in Gwei.

Copy link
Contributor

@james-prysm james-prysm left a comment

Choose a reason for hiding this comment

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

ok with this but lets also wait for thoughts from terence

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

lgtm - I still have some hesitations on tracking two values vs one value, but it's not a show stopper. Can move on

@rkapka rkapka added this pull request to the merge queue Jan 24, 2024
Merged via the queue into develop with commit c996109 Jan 24, 2024
17 checks passed
@rkapka rkapka deleted the produce-block-wei branch January 24, 2024 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/eth/v3/validator/blocks/{slot} not returning execution payload value
3 participants