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

Greg/validator/rpc #185

Merged
merged 12 commits into from
May 5, 2019
Merged

Greg/validator/rpc #185

merged 12 commits into from
May 5, 2019

Conversation

GregTheGreek
Copy link
Member

Loosely implemented off of ethereum/consensus-specs#1011

Also includes a refactor of the rpc/api section. Added two directories modules and interfaces where we can keep our classes that contain the logic.

What this doesn't include is name spacing of the api methods. I'll make an issue and do that next.

@GregTheGreek GregTheGreek requested a review from a team May 2, 2019 15:38
@codecov
Copy link

codecov bot commented May 2, 2019

Codecov Report

Merging #185 into master will increase coverage by 0.08%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   31.85%   31.93%   +0.08%     
==========================================
  Files          46       50       +4     
  Lines         744      811      +67     
  Branches       64       70       +6     
==========================================
+ Hits          237      259      +22     
- Misses        507      552      +45

src/rpc/protocol/jsonRpc.ts Outdated Show resolved Hide resolved
src/rpc/protocol/jsonRpc.ts Outdated Show resolved Hide resolved
src/rpc/api/modules/validator.ts Outdated Show resolved Hide resolved
return {} as BeaconBlock;
}

public async produceAttestation(slot: Slot, shard: Shard): Promise<IndexedAttestation> {
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to return an AttestationData, see getAttestationData of the BeaconApi

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you sure? IndexedAttestation contains AttestationData

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, now I see the issue, opPool.receiveAttestaion looks for AttestationData

Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at https://github.com/ethereum/eth2.0-specs/blob/dev/specs/validator/0_beacon-chain-validator.md#attestations-1

It seems like the validator is supposed to get an AttestationData and turn it into an Attestation, and then publish that.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm ok I see what you are saying -- So the beacon chain should send the validator attestationData and then the validator turns it into an IndexedAttestation
?

src/types/misc.ts Outdated Show resolved Hide resolved
src/rpc/api/interfaces/validator.ts Outdated Show resolved Hide resolved
src/rpc/api/mock/validator.ts Outdated Show resolved Hide resolved
src/rpc/api/index.ts Outdated Show resolved Hide resolved
src/rpc/api/interfaces/validator.ts Outdated Show resolved Hide resolved
src/rpc/api/mock/validator.ts Outdated Show resolved Hide resolved
src/rpc/api/mock/validator.ts Outdated Show resolved Hide resolved
src/rpc/api/modules/validator.ts Outdated Show resolved Hide resolved
return {} as BeaconBlock;
}

public async produceAttestation(slot: Slot, shard: Shard): Promise<AttestationData> {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should probably call this produceAttestationData just to be more clear

Copy link
Member Author

Choose a reason for hiding this comment

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

See i think we should probably go back to IndexedAttestation based on the gitter conversations its a lot of extra work to just send the data..

src/rpc/api/modules/validator.ts Outdated Show resolved Hide resolved
mpetrunic
mpetrunic previously approved these changes May 4, 2019
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.

💃 looks good

@mpetrunic mpetrunic merged commit 4a3d6fc into master May 5, 2019
@wemeetagain wemeetagain deleted the greg/validator/rpc branch May 5, 2019 19:09
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