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

[Feature] Prevent state reset of treasury after hard fork #318

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

yun-yeo
Copy link
Contributor

@yun-yeo yun-yeo commented Jan 22, 2020

Summary of changes

Now we introduce CumulatedHeight to keep indicator histories which are stored per each epoch even if the block height is reset (at hard fork).

When we export genesis for zero height, it will update CumulatedHeight to CumulatedHeight + ctx.BlockHeight() for next chain without deleting indicator histories.

close #317

Report of required housekeeping

  • Github issue OR spec proposal link
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]

(FOR ADMIN) Before merging

  • Added appropriate labels to PR
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Confirm added tests are consistent with the intended behavior of changes
  • Ensure all tests pass

@yun-yeo yun-yeo added enhancement New feature or request daemon daemon updates labels Jan 22, 2020
@yun-yeo yun-yeo requested a review from dokwon January 22, 2020 07:30
@yun-yeo yun-yeo self-assigned this Jan 22, 2020
Copy link
Contributor

@dokwon dokwon left a comment

Choose a reason for hiding this comment

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

Many changes are not related to the stated purpose of the PR. consider moving them to separate pull requests. Also, there is a spelling error in the PR title: pork --> fork

@@ -27,7 +27,6 @@ const (
BlocksPerWeek = util.BlocksPerWeek
BlocksPerMonth = util.BlocksPerMonth
BlocksPerYear = util.BlocksPerYear
BlocksPerEpoch = util.BlocksPerEpoch
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing blocksPerEpoch seems to be a big decision, also one that is quite different from the stated purpose of this pull request. Consider moving to another pull request to avoid future confusion.

@@ -36,7 +41,7 @@ func (k Keeper) alignCoins(ctx sdk.Context, coins sdk.DecCoins, denom string) (a

// UpdateIndicators updates interal indicators
func (k Keeper) UpdateIndicators(ctx sdk.Context) {
epoch := core.GetEpoch(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also seems like code changes that are not relevant to treasury data reset

Copy link
Contributor

@dokwon dokwon left a comment

Choose a reason for hiding this comment

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

Understood on necessary changes

@dokwon dokwon merged commit 12789aa into develop Mar 24, 2020
@dokwon dokwon deleted the feature/cumulative-epoch branch March 24, 2020 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
daemon daemon updates enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Prevent reset of treasury module after hard fork
2 participants