Skip to content

Remove unused epoch-related code - #1508

Merged
mergify[bot] merged 7 commits into
CodeChain-io:masterfrom
foriequal0:feature/remove-epoch
May 13, 2019
Merged

mergify[bot] merged 7 commits into
CodeChain-io:masterfrom
foriequal0:feature/remove-epoch

Conversation

@foriequal0

@foriequal0 foriequal0 commented May 2, 2019 •

Copy link
Copy Markdown
Contributor

This patch should not introduce breaking-change.
I'll check that by synchronizing with the testnet.
Resolve: #1507

`EpochChange::{Yes,Unsure}` is never constructed. Only `EpochChange::No`
is used. We can remove these codes safely since they are dead-code after
propagating constant.
@foriequal0
foriequal0 requested a review from sgkim126 May 2, 2019 09:27
@sgkim126
sgkim126 requested review from majecty and removed request for sgkim126 May 2, 2019 11:22
@sgkim126

sgkim126 commented May 2, 2019

Copy link
Copy Markdown
Contributor

@majecty Would you review it? I think you see these code more than me.

@majecty

majecty commented May 3, 2019

Copy link
Copy Markdown

@foriequal0 Could you give some brief explanation about the code structure?
Like which part of epoch code are survived, and what they are doing now.

@majecty

majecty commented May 3, 2019

Copy link
Copy Markdown

LGTM.
It seems no behavior is changed.

majecty
majecty previously approved these changes May 3, 2019
@majecty majecty added the do-not-merge Do not merge (for mergify.io) label May 3, 2019
@majecty

majecty commented May 3, 2019

Copy link
Copy Markdown

Please remove the "do-not-merge" label after your progress is done.

@foriequal0

foriequal0 commented May 3, 2019 •

Copy link
Copy Markdown
Contributor Author

I've checked that

  • sync with corgi on master -> continue to sync on feature/remove-epoch
  • sync with corgi on feature/remove-epoch -> continue to sync on master

I think we should prepare a CI environment that tests the compatibility.

@foriequal0 foriequal0 changed the title [WIP] Remove unused epoch-related code Remove unused epoch-related code May 3, 2019
@foriequal0
foriequal0 force-pushed the feature/remove-epoch branch from 34b7e52 to 585688d Compare May 7, 2019 09:27
@foriequal0

Copy link
Copy Markdown
Contributor Author

@majecty I've appended more removal commits.
Could you review 4 commits from Remove unused is_epoch_begin to Remove unused insert_epoch_transition?
It shouldn't introduce any breaking changes too.
I've check that the sync is compatable between master and the head of the PR.

@foriequal0
foriequal0 requested a review from majecty May 7, 2019 09:31
majecty
majecty previously approved these changes May 8, 2019

@majecty majecty left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

foriequal0 added 5 commits May 8, 2019 21:40
`insert_pending_transition` is not used anymore after removing
`EpochChange`. `get_pending_transition` should alway return `None` since
there are no inserted `PendingTransition`s.
`ConsensusEngine::epoch_verifier` is not used anywhere.
Constant propagate and remove is_epoch_end
@foriequal0

foriequal0 commented May 9, 2019 •

Copy link
Copy Markdown
Contributor Author

@majecty Sorry for the frequent changes.
I've separated last 4 commits into #1519 and rebased them.

@majecty majecty left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@foriequal0 foriequal0 removed the do-not-merge Do not merge (for mergify.io) label May 13, 2019
@mergify
mergify Bot merged commit 00195ea into CodeChain-io:master May 13, 2019
@foriequal0
foriequal0 deleted the feature/remove-epoch branch May 13, 2019 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove unused epoch-related code

3 participants