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

Jump to bottom

chore: replace constructor of EpochExt by builder #1542

Merged
merged 1 commit into from Sep 5, 2019
Merged

chore: replace constructor of EpochExt by builder #1542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 10 additions & 10 deletions script/src/syscalls/mod.rs
View file
Open in desktop
Expand Up @@ -656,16 +656,16 @@ mod tests {
.transactions_root(data_hash.pack())
.build();

let epoch = EpochExt::new(
u64::from(data[0]),
Capacity::bytes(100).unwrap(),
Capacity::bytes(100).unwrap(),
U256::one(),
Byte32::default(),
1234,
1000,
U256::one(),
);
let epoch = EpochExt::new_builder()
.number(u64::from(data[0]))
.base_block_reward(Capacity::bytes(100).unwrap())
.remainder_reward(Capacity::bytes(100).unwrap())
.previous_epoch_hash_rate(U256::one())
.last_block_hash_in_previous_epoch(Byte32::default())
.start_number(1234)
.length(1000)
.difficulty(U256::one())
.build();

let mut correct_data = [0u8; 8];
LittleEndian::write_u64(&mut correct_data, epoch.number());
Expand Down
40 changes: 20 additions & 20 deletions spec/src/consensus.rs
View file
Open in desktop
Expand Up @@ -165,16 +165,16 @@ impl Consensus {
* (GENESIS_EPOCH_LENGTH + GENESIS_ORPHAN_COUNT)
/ EPOCH_DURATION_TARGET;

let genesis_epoch_ext = EpochExt::new(
0, // number
block_reward, // block_reward
remainder_reward, // remainder_reward
genesis_hash_rate,
Byte32::zero(),
0, // start
GENESIS_EPOCH_LENGTH, // length
genesis_header.difficulty(), // difficulty,
);
let genesis_epoch_ext = EpochExt::new_builder()
.number(0)
.base_block_reward(block_reward)
.remainder_reward(remainder_reward)
.previous_epoch_hash_rate(genesis_hash_rate)
.last_block_hash_in_previous_epoch(Byte32::zero())
.start_number(0)
.length(GENESIS_EPOCH_LENGTH)
.difficulty(genesis_header.difficulty())
.build();

Consensus {
genesis_hash: genesis_header.hash(),
Expand Down Expand Up @@ -508,16 +508,16 @@ impl Consensus {
let block_reward = Capacity::shannons(self.epoch_reward().as_u64() / next_epoch_length);
let remainder_reward = Capacity::shannons(self.epoch_reward().as_u64() % next_epoch_length);

let epoch_ext = EpochExt::new(
last_epoch.number() + 1, // number
block_reward,
remainder_reward, // remainder_reward
adjusted_last_epoch_hash_rate,
header.hash(), // last_block_hash_in_previous_epoch
header_number + 1, // start
next_epoch_length, // length
next_epoch_diff, // difficulty,
);
let epoch_ext = EpochExt::new_builder()
.number(last_epoch.number() + 1)
.base_block_reward(block_reward)
.remainder_reward(remainder_reward)
.previous_epoch_hash_rate(adjusted_last_epoch_hash_rate)
.last_block_hash_in_previous_epoch(header.hash())
.start_number(header_number + 1)
.length(next_epoch_length)
.difficulty(next_epoch_diff)
.build();

Some(epoch_ext)
}
Expand Down
20 changes: 10 additions & 10 deletions util/dao/src/lib.rs
View file
Open in desktop
Expand Up @@ -335,16 +335,16 @@ mod tests {
.build();
// TODO: should make it simple after refactor get_ancestor
for number in target_epoch_start..parent.number() {
let epoch_ext = EpochExt::new(
number,
Capacity::shannons(50_000_000_000),
Capacity::shannons(1_000_128),
U256::one(),
h256!("0x1").pack(),
target_epoch_start,
2091,
U256::from(1u64),
);
let epoch_ext = EpochExt::new_builder()
.number(number)
.base_block_reward(Capacity::shannons(50_000_000_000))
.remainder_reward(Capacity::shannons(1_000_128))
.previous_epoch_hash_rate(U256::one())
.last_block_hash_in_previous_epoch(h256!("0x1").pack())
.start_number(target_epoch_start)
.length(2091)
.difficulty(U256::one())
.build();

let header = HeaderBuilder::default()
.number(number.pack())
Expand Down
2 changes: 1 addition & 1 deletion util/types/schemas/ckb.mol
View file
Open in desktop
Expand Up @@ -143,7 +143,7 @@ table EpochExt {
last_block_hash_in_previous_epoch: Byte32,
difficulty: Byte32,
number: Uint64,
block_reward: Uint64,
base_block_reward: Uint64,
remainder_reward: Uint64,
start_number: Uint64,
length: Uint64,
Expand Down
4 changes: 2 additions & 2 deletions util/types/src/conversion/storage.rs
View file
Open in desktop
Expand Up @@ -88,7 +88,7 @@ impl Pack<packed::EpochExt> for core::EpochExt {
fn pack(&self) -> packed::EpochExt {
packed::EpochExt::new_builder()
.number(self.number().pack())
.block_reward(self.base_block_reward().pack())
.base_block_reward(self.base_block_reward().pack())
.remainder_reward(self.remainder_reward().pack())
.previous_epoch_hash_rate(self.previous_epoch_hash_rate().pack())
.last_block_hash_in_previous_epoch(self.last_block_hash_in_previous_epoch().clone())
Expand All @@ -103,7 +103,7 @@ impl<'r> Unpack<core::EpochExt> for packed::EpochExtReader<'r> {
fn unpack(&self) -> core::EpochExt {
core::EpochExt {
number: self.number().unpack(),
block_reward: self.block_reward().unpack(),
base_block_reward: self.base_block_reward().unpack(),
remainder_reward: self.remainder_reward().unpack(),
previous_epoch_hash_rate: self.previous_epoch_hash_rate().unpack(),
last_block_hash_in_previous_epoch: self.last_block_hash_in_previous_epoch().to_entity(),
Expand Down
205 changes: 127 additions & 78 deletions util/types/src/core/extras.rs
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ impl TransactionInfo {
#[derive(Clone, Eq, PartialEq, Debug, Default)]
pub struct EpochExt {
pub(crate) number: EpochNumber,
pub(crate) block_reward: Capacity,
pub(crate) base_block_reward: Capacity,
pub(crate) remainder_reward: Capacity,
pub(crate) previous_epoch_hash_rate: U256,
pub(crate) last_block_hash_in_previous_epoch: packed::Byte32,
Expand All @@ -64,29 +64,32 @@ pub struct EpochExt {
pub(crate) difficulty: U256,
}

#[derive(Clone, Debug)]
pub struct EpochExtBuilder(pub(crate) EpochExt);

impl EpochExt {
pub fn number(&self) -> u64 {
//
// Simple Getters
//

pub fn number(&self) -> BlockNumber {
self.number
}

pub fn block_reward(&self, number: BlockNumber) -> Result<Capacity, FailureError> {
if number >= self.start_number()
&& number < self.start_number() + self.remainder_reward.as_u64()
{
self.block_reward
.safe_add(Capacity::one())
.map_err(Into::into)
} else {
Ok(self.block_reward)
}
pub fn base_block_reward(&self) -> &Capacity {
&self.base_block_reward
}

pub fn base_block_reward(&self) -> &Capacity {
&self.block_reward
pub fn remainder_reward(&self) -> &Capacity {
&self.remainder_reward
}

pub fn is_genesis(&self) -> bool {
0 == self.number
pub fn previous_epoch_hash_rate(&self) -> &U256 {
&self.previous_epoch_hash_rate
}

pub fn last_block_hash_in_previous_epoch(&self) -> packed::Byte32 {
self.last_block_hash_in_previous_epoch.clone()
}

pub fn start_number(&self) -> BlockNumber {
Expand All @@ -97,6 +100,41 @@ impl EpochExt {
self.length
}

pub fn difficulty(&self) -> &U256 {
&self.difficulty
}

//
// Simple Setters
//

pub fn set_number(&mut self, number: BlockNumber) {
self.number = number;
}

pub fn set_base_block_reward(&mut self, base_block_reward: Capacity) {
self.base_block_reward = base_block_reward;
}

pub fn set_remainder_reward(&mut self, remainder_reward: Capacity) {
self.remainder_reward = remainder_reward;
}

pub fn set_previous_epoch_hash_rate(&mut self, previous_epoch_hash_rate: U256) {
self.previous_epoch_hash_rate = previous_epoch_hash_rate;
}

pub fn set_last_block_hash_in_previous_epoch(
&mut self,
last_block_hash_in_previous_epoch: packed::Byte32,
) {
self.last_block_hash_in_previous_epoch = last_block_hash_in_previous_epoch;
}

pub fn set_start_number(&mut self, start_number: BlockNumber) {
self.start_number = start_number;
}

pub fn set_length(&mut self, length: BlockNumber) {
self.length = length;
}
Expand All @@ -105,80 +143,91 @@ impl EpochExt {
self.difficulty = difficulty;
}

pub fn difficulty(&self) -> &U256 {
&self.difficulty
//
// Normal Methods
//

pub fn new_builder() -> EpochExtBuilder {
EpochExtBuilder(EpochExt::default())
}

pub fn remainder_reward(&self) -> &Capacity {
&self.remainder_reward
pub fn into_builder(self) -> EpochExtBuilder {
EpochExtBuilder(self)
}

pub fn last_block_hash_in_previous_epoch(&self) -> packed::Byte32 {
self.last_block_hash_in_previous_epoch.clone()
pub fn is_genesis(&self) -> bool {
0 == self.number
}

pub fn previous_epoch_hash_rate(&self) -> &U256 {
&self.previous_epoch_hash_rate
pub fn block_reward(&self, number: BlockNumber) -> Result<Capacity, FailureError> {
if number >= self.start_number()
&& number < self.start_number() + self.remainder_reward.as_u64()
{
self.base_block_reward
.safe_add(Capacity::one())
.map_err(Into::into)
} else {
Ok(self.base_block_reward)
}
}
}

impl EpochExtBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the builder is redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't get your point.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

//
// Simple Setters
//

pub fn set_previous_epoch_hash_rate(&mut self, hash_rate: U256) {
self.previous_epoch_hash_rate = hash_rate
pub fn number(mut self, number: BlockNumber) -> Self {
self.0.set_number(number);
self
}

#[allow(clippy::too_many_arguments)]
pub fn new(
number: u64,
block_reward: Capacity,
remainder_reward: Capacity,
previous_epoch_hash_rate: U256,
pub fn base_block_reward(mut self, base_block_reward: Capacity) -> Self {
self.0.set_base_block_reward(base_block_reward);
self
}

pub fn remainder_reward(mut self, remainder_reward: Capacity) -> Self {
self.0.set_remainder_reward(remainder_reward);
self
}

pub fn previous_epoch_hash_rate(mut self, previous_epoch_hash_rate: U256) -> Self {
self.0
.set_previous_epoch_hash_rate(previous_epoch_hash_rate);
self
}

pub fn last_block_hash_in_previous_epoch(
mut self,
last_block_hash_in_previous_epoch: packed::Byte32,
start_number: BlockNumber,
length: BlockNumber,
difficulty: U256,
) -> EpochExt {
EpochExt {
number,
block_reward,
remainder_reward,
previous_epoch_hash_rate,
last_block_hash_in_previous_epoch,
start_number,
length,
difficulty,
}
) -> Self {
self.0
.set_last_block_hash_in_previous_epoch(last_block_hash_in_previous_epoch);
self
}

pub fn destruct(
self,
) -> (
u64,
Capacity,
Capacity,
U256,
packed::Byte32,
BlockNumber,
BlockNumber,
U256,
) {
let EpochExt {
number,
block_reward,
remainder_reward,
previous_epoch_hash_rate,
last_block_hash_in_previous_epoch,
start_number,
length,
difficulty,
} = self;
(
number,
block_reward,
remainder_reward,
previous_epoch_hash_rate,
last_block_hash_in_previous_epoch,
start_number,
length,
difficulty,
)
pub fn start_number(mut self, start_number: BlockNumber) -> Self {
self.0.set_start_number(start_number);
self
}

pub fn length(mut self, length: BlockNumber) -> Self {
self.0.set_length(length);
self
}

pub fn difficulty(mut self, difficulty: U256) -> Self {
self.0.set_difficulty(difficulty);
self
}

//
// Normal Methods
//
// The `new` methods are unnecessary. Creating `EpochExtBuilder` from `EpochExt`, it's enough.

pub fn build(self) -> EpochExt {
self.0
}
}