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

Unstub validator rpc #188

Closed
wants to merge 2 commits into from
Closed

Unstub validator rpc #188

wants to merge 2 commits into from

Conversation

mpetrunic
Copy link
Member

I started unstubbing validator rpc calls.
I'm not really sure how much of that will end up in beacon/validator api in beacon node so I've put abstract class that implements all those data processing functions that validator require.

I would leave them for now like this until rpc endpoints are defined and then we can either move this logic from AbstractClient to beacon node. If they are going to stay in validator it should probably be extracted to some class with more sane name.

I would like if you could check if I implemented those things correctly before I start writing tests.

@GregTheGreek
Copy link
Member

GregTheGreek commented May 3, 2019

Why did you re-implement all the functions? If anything you could import them from the beacon chain since its in the same project.

Also - check out my pr #185 because the RPC methods you'll need are a lot lighter

@mpetrunic
Copy link
Member Author

Why did you re-implement all the functions? If anything you could import them from the beacon chain since its in the same project.

Also - check out my pr #185 because the RPC methods you'll need are a lot lighter

TBH I failed to find some of this methods in chain folder, my bad (was late when I was searching) :(

Do you plan exposing get_committee_assignment as validator rpc or should we calculate it in validator code? Depending on that I will either change to import from chain directory or make a call to API.

@GregTheGreek
Copy link
Member

Why did you re-implement all the functions? If anything you could import them from the beacon chain since its in the same project.
Also - check out my pr #185 because the RPC methods you'll need are a lot lighter

TBH I failed to find some of this methods in chain folder, my bad (was late when I was searching) :(

Ahhh dont worry

Do you plan exposing get_committee_assignment as validator rpc or should we calculate it in validator code? Depending on that I will either change to import from chain directory or make a call to API.

Well I think the validator wouldn't need that because the assingment would be passed in via the get_duties rpc call

@mpetrunic
Copy link
Member Author

Well I think the validator wouldn't need that because the assingment would be passed in via the get_duties rpc call

My chain of thought was to offload some of the computing from beacon node as all these stuff can be calculated from BeaconState.

@GregTheGreek
Copy link
Member

My chain of thought was to offload some of the computing from beacon node as all these stuff can be calculated from BeaconState.

Hmm.... I see what you're saying, I guess it matters on how "stateless" we want to be.

Also bare in mind that #185 is loosely implemented off of ethereum/consensus-specs#1011

@mpetrunic
Copy link
Member Author

mpetrunic commented May 4, 2019

How about this interface for validator client?
I guess if we use produceBlock and produceAttestation, I could replace onNewBeaconState and onNewAttestationdata with onNewValidatorDuty?
@GregTheGreek @wemeetagain
I will hold on to implementation till we close rpc PR. :)

@GregTheGreek
Copy link
Member

Yeah lets take a back seat on this, I'm changing the api again

@GregTheGreek
Copy link
Member

This is probably good to finish

@wemeetagain wemeetagain mentioned this pull request May 6, 2019
33 tasks
@mpetrunic mpetrunic closed this May 8, 2019
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