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

feat: fix epoch params #103

Merged
merged 24 commits into from
Sep 16, 2021
Merged

Conversation

jaybxyz
Copy link
Contributor

@jaybxyz jaybxyz commented Sep 13, 2021

Description

This PR is implemented by following the approach 1 (See the issue #96).
It adds CurrentEpochDays global parameter for key store prefix and use it in EndBlocker to handle reward distribution.

closes: #96

Tasks

  • Add CurrentEpochDays (approach 1) or EpochEra (approach 2)
  • Rename EpochDays to NextEpochDays in params and fix logics related to it
  • Update spec and description
  • Write test codes

Additional works


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Appropriate labels applied
  • Targeted PR against correct branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@jaybxyz jaybxyz added the enhancement New feature or request label Sep 13, 2021
@jaybxyz jaybxyz self-assigned this Sep 13, 2021
@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #103 (33b41ad) into master (1e6f946) will increase coverage by 3.23%.
The diff coverage is 71.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   66.36%   69.60%   +3.23%     
==========================================
  Files          24       25       +1     
  Lines        2224     2260      +36     
==========================================
+ Hits         1476     1573      +97     
+ Misses        608      534      -74     
- Partials      140      153      +13     
Impacted Files Coverage Δ
x/farming/keeper/grpc_query.go 83.21% <0.00%> (-3.65%) ⬇️
x/farming/types/keys.go 71.23% <ø> (+71.23%) ⬆️
x/farming/types/utils.go 60.00% <60.00%> (ø)
x/farming/keeper/epoch.go 72.50% <68.75%> (-2.50%) ⬇️
x/farming/abci.go 66.66% <100.00%> (+66.66%) ⬆️
x/farming/keeper/genesis.go 80.00% <100.00%> (+0.53%) ⬆️
x/farming/keeper/staking.go 90.95% <100.00%> (+0.04%) ⬆️
x/farming/types/genesis.go 55.00% <100.00%> (+2.36%) ⬆️
x/farming/types/params.go 80.39% <100.00%> (ø)
... and 3 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 1e6f946...33b41ad. Read the comment docs.

@dongsam
Copy link
Contributor

dongsam commented Sep 14, 2021

#98 just merged, so It would be better to merge master first

@jaybxyz jaybxyz marked this pull request as ready for review September 15, 2021 13:08
Copy link
Contributor

@dongsam dongsam left a comment

Choose a reason for hiding this comment

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

If it seems like it's going to end soon, I think it'd be nice to add query and cli for Current EpochDays.

x/farming/keeper/genesis.go Outdated Show resolved Hide resolved
x/farming/keeper/epoch.go Outdated Show resolved Hide resolved
x/farming/spec/08_params.md Show resolved Hide resolved
x/farming/abci_test.go Outdated Show resolved Hide resolved
@jaybxyz jaybxyz requested a review from dongsam September 16, 2021 07:18
Copy link
Contributor

@dongsam dongsam left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@hallazzang hallazzang left a comment

Choose a reason for hiding this comment

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

Great work. Some changes requested

proto/tendermint/farming/v1beta1/farming.proto Outdated Show resolved Hide resolved
x/farming/keeper/msg_server.go Outdated Show resolved Hide resolved
x/farming/spec/03_state_transitions.md Outdated Show resolved Hide resolved
x/farming/spec/03_state_transitions.md Outdated Show resolved Hide resolved
x/farming/types/genesis.go Outdated Show resolved Hide resolved
x/farming/types/params.go Outdated Show resolved Hide resolved
x/farming/types/params_test.go Show resolved Hide resolved
…96-fix-epoch-params

# Conflicts:
#	proto/tendermint/farming/v1beta1/query.proto
#	x/farming/keeper/grpc_query.go
#	x/farming/types/farming.pb.go
#	x/farming/types/query.pb.go
#	x/farming/types/query.pb.gw.go
…96-fix-epoch-params

# Conflicts:
#	proto/tendermint/farming/v1beta1/genesis.proto
#	x/farming/client/cli/query.go
#	x/farming/keeper/epoch_test.go
#	x/farming/keeper/genesis.go
#	x/farming/keeper/keeper_test.go
#	x/farming/types/farming.pb.go
#	x/farming/types/genesis.go
#	x/farming/types/genesis.pb.go
@dongsam dongsam merged commit f03a6b8 into tendermint:master Sep 16, 2021
jaybxyz added a commit to jaybxyz/farming that referenced this pull request Sep 17, 2021
* master:
  feat: add budget module on simapp (tendermint#118)
  feat: fix epoch params (tendermint#103)
  chore: fix typo
  test: add TestOutstandingRewards
  style: sort imports
  feat: add AdvanceEpoch message and tx cli endpoint
  feat: add cli query commands
  test: write test for Rewards query endpoint
  test: write test for TotalStaking query endpoint
  test: write test for Staking query endpoint
  feat: implement new query endpoints
  docs: update spec document
  fix: include OutstandingRewards in genesis
  fix: fix epoch advancing criteria
  fix: use sdk.DecCoins for rewards
  fix: set total staking on InitGenesis and use helper methods
  feat: add OutstandingRewards
  chore: remove unnecessary validation from InitGenesis
  fix: fix InitGenesis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: Move mustParseRFC3339 function to utils Fix params.EpochDays to params.NextEpochDays
3 participants