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.

Sign up for GitHub

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

Jump to bottom

Expose staking event hashes + add GetGenesisDocument to consensus client API #2889

Merged
merged 2 commits into from
May 6, 2020
Merged

Expose staking event hashes + add GetGenesisDocument to consensus client API #2889

merged 2 commits into from
May 6, 2020

Conversation

abukosek
Copy link
Contributor

@abukosek abukosek commented May 6, 2020

No description provided.

@abukosek abukosek force-pushed the andrej/feature/expose-event-hashes-and-genesis-doc branch 2 times, most recently from 3ba7b09 to e2b4e88 Compare May 6, 2020 09:00
kostko
kostko reviewed May 6, 2020
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Consider describing the changes in the changelog fragment, also I think they should be features as they change the external API.

go/staking/api/api.go Outdated Show resolved Hide resolved
go/consensus/tendermint/tendermint.go Outdated Show resolved Hide resolved
go/consensus/tendermint/tendermint.go
@@ -446,6 +446,24 @@ func (t *tendermintService) StateToGenesis(ctx context.Context, blockHeight int6
}, nil
}

func (t *tendermintService) GetGenesisDocument(ctx context.Context) (*genesisAPI.Document, error) {
Copy link
Member

@kostko kostko May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Don't forget to remove the existing GetGenesis method that exists in the tendermint-specific service (any callers should use this new one).

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right thing to name the method? It's a state dump, not a genesis document (#2886 is illustrative of the fact that there is a difference).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After the suggested changes, GetGenesisDocument returns t.genesis, so it should be equivalent to the old GetGenesis method.

go/consensus/tendermint/staking/staking.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 6, 2020 •
edited
Loading

Codecov Report

Merging #2889 into master will increase coverage by 0.27%.
The diff coverage is 78.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2889      +/-   ##
==========================================
+ Coverage   67.69%   67.97%   +0.27%     
==========================================
  Files         352      352              
  Lines       34188    34238      +50     
==========================================
+ Hits        23145    23274     +129     
+ Misses       8069     8003      -66     
+ Partials     2974     2961      -13
Impacted Files Coverage Δ
go/consensus/api/api.go 50.00% <ø> (ø)
go/staking/api/api.go 71.42% <ø> (ø)
go/consensus/tendermint/epochtime/epochtime.go 85.33% <50.00%> (-2.17%) ⬇️
...sensus/tendermint/epochtime_mock/epochtime_mock.go 72.78% <50.00%> (-0.83%) ⬇️
go/consensus/api/grpc.go 65.57% <71.42%> (+0.35%) ⬆️
go/consensus/tendermint/tendermint.go 67.90% <75.00%> (+0.64%) ⬆️
go/consensus/tendermint/staking/staking.go 65.28% <84.09%> (+2.43%) ⬆️
go/consensus/tests/tester.go 96.00% <100.00%> (+0.25%) ⬆️
go/staking/tests/tester.go 95.90% <100.00%> (+<0.01%) ⬆️
go/consensus/tendermint/api/api.go 74.54% <0.00%> (-14.55%) ⬇️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b916601...886fc48. Read the comment docs.

@abukosek abukosek force-pushed the andrej/feature/expose-event-hashes-and-genesis-doc branch from e2b4e88 to 55e72c7 Compare May 6, 2020 09:31
@abukosek
Copy link
Contributor Author

abukosek commented May 6, 2020

Thanks for the review comments, I've addressed them now, so this is ready for re-review :)

@abukosek abukosek marked this pull request as ready for review May 6, 2020 09:43
@abukosek abukosek force-pushed the andrej/feature/expose-event-hashes-and-genesis-doc branch from 55e72c7 to 5fd0773 Compare May 6, 2020 09:46
kostko
kostko reviewed May 6, 2020
View reviewed changes
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Add tests for the new consensus method (GetGenesisDocument) to go/consensus/tests/tester.go. Also consider testing somewhere that events contain proper transaction hashes (e.g. in the same place where you test GetEvents).

go/staking/api/api.go Outdated Show resolved Hide resolved
@abukosek abukosek force-pushed the andrej/feature/expose-event-hashes-and-genesis-doc branch 3 times, most recently from 3162169 to 45d3577 Compare May 6, 2020 11:03
@abukosek
Copy link
Contributor Author

abukosek commented May 6, 2020

Done! :)

kostko
kostko reviewed May 6, 2020
View reviewed changes
go/consensus/tendermint/staking/staking.go Outdated Show resolved Hide resolved
go/consensus/tendermint/staking/staking.go Outdated Show resolved Hide resolved
@abukosek abukosek force-pushed the andrej/feature/expose-event-hashes-and-genesis-doc branch from 45d3577 to 886fc48 Compare May 6, 2020 11:50
@abukosek
Copy link
Contributor Author

abukosek commented May 6, 2020

Thanks! Ready for re-review.

@abukosek abukosek merged commit 5094a27 into master May 6, 2020
@abukosek abukosek deleted the andrej/feature/expose-event-hashes-and-genesis-doc branch May 6, 2020 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yawning Yawning Yawning left review comments

@kostko kostko kostko approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

@pro-wh pro-wh Awaiting requested review from pro-wh pro-wh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abukosek @kostko @Yawning