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

Implement GetDepositContract in the config API #8316

Merged
merged 16 commits into from
Jan 26, 2021

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Jan 22, 2021

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR implements config API's GetDepositContract method according to the spec: https://ethereum.github.io/eth2.0-APIs/#/Config/getDepositContract

Which issues(s) does this PR fix?

Part of #7510

Other notes for review

N/A

@rkapka rkapka requested a review from a team as a code owner January 22, 2021 12:44
@rkapka rkapka changed the title Contact address config api Implement GetDepositContract in the config API Jan 22, 2021
@rkapka rkapka added API Api related tasks Ready For Review labels Jan 22, 2021
Comment on lines 37 to 40
chainId, err := binary.ReadUvarint(buf)
if err != nil {
return nil, status.Errorf(codes.Internal, "could not obtain genesis fork version: %v", err)
}
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 did not test the error case as this can only happen when our fork version is completely broken.

if err != nil {
return nil, status.Errorf(codes.Internal, "could not obtain genesis fork version: %v", err)
}
byteAddress := [20]byte(bs.PowchainInfoFetcher.DepositContractAddress())
Copy link
Member

Choose a reason for hiding this comment

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

Similar to my last feedback, we do not need to implement a new service getter to obtain deposit contract address. This is already accessible via params.BeaconConfig().DepositContractAddress (where powchain service gets it too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I have to go through the config file and make a mental note of what is available.

@rkapka
Copy link
Contributor Author

rkapka commented Jan 25, 2021

Actually it turned out I tried to fetch the wrong thing. It was supposed to be the Eth1 chain ID, not the Eth2 fork version. There was a mistake in the endpoint's description. I fixed it in one of the commits.

@@ -32,6 +32,7 @@ type Server struct {
ChainInfoFetcher blockchain.ChainInfoFetcher
DepositFetcher depositcache.DepositFetcher
BlockFetcher powchain.POWBlockFetcher
PowchainInfoFetcher powchain.ChainInfoFetcher
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes still relevant to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. +1

@prylabs-bulldozer prylabs-bulldozer bot merged commit a7345c1 into develop Jan 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the contact-address-config-api branch January 26, 2021 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants