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

chore: replace constructor of EpochExt by builder #1542

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@yangby-cryptape
Copy link
Contributor

commented Sep 4, 2019

  • The constructor is too complex, it's difficult to review when we just see the invocation of new(...)
  • The field block_reward is base_block_reward in actual, so rename it to the corrected name.
@yangby-cryptape yangby-cryptape requested a review from nervosnetwork/ckb-code-review as a code owner Sep 4, 2019
@nervos-bot

This comment has been minimized.

Copy link

commented Sep 4, 2019

@doitian is assigned as the chief reviewer

}
}

impl EpochExtBuilder {

This comment has been minimized.

Copy link
@u2

u2 Sep 4, 2019

Collaborator

I think the builder is redundant

This comment has been minimized.

Copy link
@yangby-cryptape

yangby-cryptape Sep 4, 2019

Author Contributor

I didn't get your point.

IMHO, I think we should remove all setters from EpochExt.

This comment has been minimized.

Copy link
@yangby-cryptape

yangby-cryptape Sep 4, 2019

Author Contributor

See this:

ckb/spec/src/consensus.rs

Lines 211 to 218 in 87c6912

self.genesis_epoch_ext
.set_difficulty(genesis_block.difficulty());
self.genesis_hash = genesis_block.hash();
self.genesis_block = genesis_block;
self
}
pub fn set_genesis_epoch_ext(mut self, genesis_epoch_ext: EpochExt) -> Self {

If we calling set_genesis_block before calling set_genesis_epoch_ext, a bug was created.

ckb/spec/src/lib.rs

Lines 189 to 190 in ecd88dc

.set_genesis_epoch_ext(genesis_epoch_ext)
.set_genesis_block(genesis_block)

Two independent methods have to be used in a specific order.

I think we can remove all setters from EpochExt to avoid that. (We can do it after ConsensusBuilder, the ConsensusBuilder is on the task list.)
If u want to update a EpochExt, u can do ext.into_builder().set_xxx().build().

When rebuild a Consensus, then some parameters have to compute again, so we should disallow the direct setter.

@doitian

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

I also touched this in the PR to half primary reward. The issue is the set_length method, which really changes the epoch reward.

https://github.com/nervosnetwork/ckb/pull/1537/files#diff-6ded9a204ae8172088a45152773c9df6R106

@yangby-cryptape

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

I also touched this in the PR to half primary reward. The issue is the set_length method, which really changes the epoch reward.

https://github.com/nervosnetwork/ckb/pull/1537/files#diff-6ded9a204ae8172088a45152773c9df6R106

So, I think, we should (base on your PR):

  • remove all setters from EpochExt
  • If anyone want to change fields of EpochExt, just rebuild it from EpochExtBuilder.

set_length has side effects. We can put those logic into Builder.

@doitian doitian added this to 👀 Awaiting review in CKB Pull Requests Sep 5, 2019
@doitian
doitian approved these changes Sep 5, 2019
@u2
u2 approved these changes Sep 5, 2019
CKB Pull Requests automation moved this from 👀 Awaiting review to ✅ Reviewer approved Sep 5, 2019
@u2

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2019

bors r=doitian,u2

bors bot added a commit that referenced this pull request Sep 5, 2019
Merge #1542
1542: chore: replace constructor of EpochExt by builder r=doitian,u2 a=yangby-cryptape

- The constructor is too complex, it's difficult to review when we just see the invocation of `new(...)`
- The field `block_reward` is `base_block_reward` in actual, so rename it to the corrected name.

Co-authored-by: Boyu Yang <yangby@cryptape.com>
@bors

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Build succeeded

  • continuous-integration/travis-ci/push
@bors bors bot merged commit 0507af0 into nervosnetwork:develop Sep 5, 2019
4 of 5 checks passed
4 of 5 checks passed
nervosnetwork.ckb Build #20190904.34 failed
Details
Dummy CI CI that does nothing
Details
Travis CI - Pull Request Build Passed
Details
bors Build succeeded
Details
nervosnetwork.ckb (UnitTest) UnitTest succeeded
Details
CKB Pull Requests automation moved this from ✅ Reviewer approved to Done Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.