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

Add ConsensusParameters methods for scheduler/epochtime #3565

Merged
merged 5 commits into from
Dec 14, 2020

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Dec 11, 2020

TODO:

  • add test case for latest heights fix

@ptrus ptrus changed the title Add GenesisParameters methods for scheduler/epochtime Add ConsensusParameters methods for scheduler/epochtime Dec 11, 2020
@ptrus ptrus force-pushed the ptrus/fix/txsource-params-query branch 2 times, most recently from 8a0a451 to 94c7bd6 Compare December 11, 2020 12:04
Comment on lines 46 to 47
// Consensus are the backend agnostic consensus parameters.
Consensus genesis.Parameters `json:"consensus"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Consensus are the backend agnostic consensus parameters.
Consensus genesis.Parameters `json:"consensus"`
// Parameters are the backend agnostic consensus parameters.
Parameters genesis.Parameters `json:"parameters"`

Also these should probably be placed above Meta.

@ptrus ptrus force-pushed the ptrus/fix/txsource-params-query branch 9 times, most recently from e129e3b to dcf12d4 Compare December 11, 2020 15:30
@ptrus ptrus marked this pull request as ready for review December 11, 2020 15:30
@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #3565 (4276b59) into master (4c51f66) will increase coverage by 0.36%.
The diff coverage is 65.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3565      +/-   ##
==========================================
+ Coverage   66.47%   66.84%   +0.36%     
==========================================
  Files         377      377              
  Lines       34469    34544      +75     
==========================================
+ Hits        22914    23091     +177     
+ Misses       8246     8154      -92     
+ Partials     3309     3299      -10     
Impacted Files Coverage Δ
go/consensus/api/api.go 14.28% <ø> (ø)
go/consensus/tendermint/seed/seed.go 68.21% <0.00%> (-0.46%) ⬇️
go/epochtime/api/api.go 45.45% <ø> (ø)
go/scheduler/api/api.go 51.51% <ø> (ø)
go/consensus/tendermint/full/full.go 63.27% <47.36%> (+0.17%) ⬆️
go/consensus/tendermint/scheduler/scheduler.go 59.70% <50.00%> (-0.62%) ⬇️
go/consensus/tendermint/full/light.go 53.19% <60.00%> (+1.84%) ⬆️
go/consensus/api/grpc.go 69.76% <62.50%> (-0.36%) ⬇️
go/scheduler/api/grpc.go 50.00% <62.50%> (+2.38%) ⬆️
go/consensus/tendermint/apps/scheduler/query.go 75.00% <100.00%> (+0.80%) ⬆️
... and 37 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 e6e4b95...4276b59. Read the comment docs.

}

tmHeight, err := t.heightToTendermintHeight(height)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Change to a switch.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah good idea 😁 done

@@ -110,6 +110,9 @@ type ClientBackend interface {
// GetEpoch returns the current epoch.
GetEpoch(ctx context.Context, height int64) (epochtime.EpochTime, error)

// EpochtimeConsensusParameters returns the epochtime consensus parameters.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe file an issue to remove this when the new beacon backend is in place? Or mention it in #3180.

Copy link
Member Author

Choose a reason for hiding this comment

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

will comment on #3180 after i merge this

@ptrus ptrus force-pushed the ptrus/fix/txsource-params-query branch 2 times, most recently from 2015e0a to 50dbf48 Compare December 14, 2020 09:46
@ptrus ptrus force-pushed the ptrus/fix/txsource-params-query branch from 50dbf48 to b304bce Compare December 14, 2020 10:17
@ptrus ptrus force-pushed the ptrus/fix/txsource-params-query branch from b304bce to 4276b59 Compare December 14, 2020 10:29
@ptrus ptrus merged commit 502125c into master Dec 14, 2020
@ptrus ptrus deleted the ptrus/fix/txsource-params-query branch December 14, 2020 10:53
@ptrus ptrus mentioned this pull request Dec 14, 2020
21 tasks
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.

None yet

2 participants