Skip to content

chain-impl-mockchain: use more idiomatic builder-style APIs #533

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 18 additions & 24 deletions chain-impl-mockchain/benches/tally.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use chain_crypto::testing::TestCryptoRng;
use chain_impl_mockchain::testing::scenario::template::WalletTemplateBuilder;
use chain_impl_mockchain::{
certificate::{
DecryptedPrivateTally, DecryptedPrivateTallyProposal, EncryptedVoteTally, VoteCast,
Expand Down Expand Up @@ -41,18 +40,15 @@ fn tally_benchmark(
) {
let mut rng = TestCryptoRng::seed_from_u64(0);

// All wallets that are needed to be initialized in the genesis block
// TODO the underlying ledger constructor is not using this &mut. This should be a plain
// Vec<WalletTemplateBuilder>, which will greatly simplify this code.
let mut wallets: Vec<&mut WalletTemplateBuilder> = Vec::new();
let mut wallets = Vec::new();

// Stake pool owner
let mut alice_wallet_builder = wallet(ALICE);
alice_wallet_builder
let alice_wallet_builder = wallet(ALICE)
.with(1_000)
.owns(STAKE_POOL)
.committee_member();
wallets.push(&mut alice_wallet_builder);

wallets.push(alice_wallet_builder);

// generate the required number of wallets from the distribution
let voters_aliases: Vec<_> = (1..=voters_count)
Expand All @@ -63,17 +59,15 @@ fn tally_benchmark(
.take(voters_count)
.collect();
let total_votes = voting_powers.iter().sum();
let mut voters_wallets: Vec<_> = voters_aliases
.iter()
.zip(voting_powers.iter())
.map(|(alias, voting_power)| {
let mut wallet_builder = WalletTemplateBuilder::new(alias);
wallet_builder.with(*voting_power);
wallet_builder
})
.collect();
{
let mut voters_wallets: Vec<_> = voters_aliases
.iter()
.zip(voting_powers.iter())
.map(|(alias, voting_power)| wallet(alias).with(*voting_power))
.collect();

wallets.append(&mut voters_wallets.iter_mut().collect());
wallets.append(&mut voters_wallets);
}

// Prepare committee members keys
let members = CommitteeMembersManager::new(&mut rng, CRS_SEED, THRESHOLD, MEMBERS_NO);
Expand All @@ -84,16 +78,16 @@ fn tally_benchmark(
.collect();

// Build the vote plan
let mut vote_plan_builder = vote_plan(VOTE_PLAN);
vote_plan_builder
let mut vote_plan_builder = vote_plan(VOTE_PLAN)
.owner(ALICE)
.consecutive_epoch_dates()
.payload_type(PayloadType::Private)
.committee_keys(committee_keys);
for _ in 0..n_proposals {
let mut proposal_builder = proposal(VoteTestGen::external_proposal_id());
proposal_builder.options(3).action_off_chain();
vote_plan_builder.with_proposal(&mut proposal_builder);
let proposal_builder = proposal(VoteTestGen::external_proposal_id())
.options(3)
.action_off_chain();
vote_plan_builder = vote_plan_builder.with_proposal(proposal_builder);
}

// Initialize ledger
Expand All @@ -104,7 +98,7 @@ fn tally_benchmark(
.with_rewards(Value(1000)),
)
.with_initials(wallets)
.with_vote_plans(vec![&mut vote_plan_builder])
.with_vote_plans(vec![vote_plan_builder])
.build()
.unwrap();

Expand Down
28 changes: 13 additions & 15 deletions chain-impl-mockchain/src/testing/builders/block_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,41 +32,39 @@ impl GenesisPraosBlockBuilder {
}
}

pub fn with_parent(&mut self, parent: &Header) -> &mut Self {
self.with_parent_id(parent.hash());
self.with_date(parent.block_date());
self.with_chain_length(parent.chain_length());
self
pub fn with_parent(self, parent: &Header) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you describe the benefits of changing this? Both styles have their own advantages.

self.with_parent_id(parent.hash())
.with_date(parent.block_date())
.with_chain_length(parent.chain_length())
}

pub fn with_parent_id(&mut self, parent_id: Hash) -> &mut Self {
pub fn with_parent_id(mut self, parent_id: Hash) -> Self {
self.parent_id = Some(parent_id);
self
}

pub fn with_date(&mut self, date: BlockDate) -> &mut Self {
pub fn with_date(mut self, date: BlockDate) -> Self {
self.date = Some(date);
self
}

pub fn with_chain_length(&mut self, chain_length: ChainLength) -> &mut Self {
pub fn with_chain_length(mut self, chain_length: ChainLength) -> Self {
self.chain_length = Some(chain_length);
self
}

pub fn with_fragment(&mut self, fragment: Fragment) -> &mut Self {
pub fn with_fragment(mut self, fragment: Fragment) -> Self {
self.contents_builder.push(fragment);
self
}

pub fn with_fragments(&mut self, fragments: Vec<Fragment>) -> &mut Self {
for fragment in fragments {
self.with_fragment(fragment);
}
self
pub fn with_fragments(self, fragments: Vec<Fragment>) -> Self {
fragments
.into_iter()
.fold(self, |builder, fragment| builder.with_fragment(fragment))
}

pub fn build(&self, stake_pool: &StakePool, time_era: &TimeEra) -> Block {
pub fn build(self, stake_pool: &StakePool, time_era: &TimeEra) -> Block {
if self.date.is_none() || self.chain_length.is_none() || self.parent_id.is_none() {
panic!("date,chain_length or hash is not set");
}
Expand Down
18 changes: 9 additions & 9 deletions chain-impl-mockchain/src/testing/builders/stake_pool_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,37 +45,37 @@ impl StakePoolBuilder {
}
}

pub fn with_owners(&mut self, owners: Vec<PublicKey<Ed25519>>) -> &mut Self {
pub fn with_owners(mut self, owners: Vec<PublicKey<Ed25519>>) -> Self {
self.owners.extend(owners);
self
}

pub fn with_alias(&mut self, alias: &str) -> &mut Self {
pub fn with_alias(mut self, alias: &str) -> Self {
self.alias = alias.to_owned();
self
}

pub fn with_operators(&mut self, operators: Vec<PublicKey<Ed25519>>) -> &mut Self {
pub fn with_operators(mut self, operators: Vec<PublicKey<Ed25519>>) -> Self {
self.operators.extend(operators);
self
}

pub fn with_pool_permissions(&mut self, permissions: PoolPermissions) -> &mut Self {
pub fn with_pool_permissions(mut self, permissions: PoolPermissions) -> Self {
self.pool_permissions = Some(permissions);
self
}

pub fn with_reward_account(&mut self, reward_account: bool) -> &mut Self {
pub fn with_reward_account(mut self, reward_account: bool) -> Self {
self.reward_account = reward_account;
self
}

pub fn with_ratio_tax_type(
&mut self,
self,
numerator: u64,
denominator: u64,
max_limit: Option<u64>,
) -> &mut Self {
) -> Self {
self.with_tax_type(TaxType {
fixed: Value(0),
ratio: Ratio {
Expand All @@ -86,12 +86,12 @@ impl StakePoolBuilder {
})
}

pub fn with_tax_type(&mut self, tax_type: TaxType) -> &mut Self {
pub fn with_tax_type(mut self, tax_type: TaxType) -> Self {
self.tax_type = tax_type;
self
}

pub fn build(&self) -> StakePool {
pub fn build(self) -> StakePool {
let mut rng = rand_core::OsRng;

let pool_vrf: KeyPair<RistrettoGroup2HashDh> = KeyPair::generate(&mut rng);
Expand Down
26 changes: 13 additions & 13 deletions chain-impl-mockchain/src/testing/builders/update_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,19 @@ impl ProposalBuilder {
config_params: ConfigParams::new(),
}
}
pub fn with_proposal_changes(&mut self, changes: Vec<ConfigParam>) -> &mut Self {
for change in changes {
self.with_proposal_change(change);
}
self

pub fn with_proposal_changes(self, changes: Vec<ConfigParam>) -> Self {
changes
.into_iter()
.fold(self, |builder, change| builder.with_proposal_change(change))
}

pub fn with_proposal_change(&mut self, change: ConfigParam) -> &mut Self {
pub fn with_proposal_change(mut self, change: ConfigParam) -> Self {
self.config_params.push(change);
self
}

pub fn build(&self) -> UpdateProposal {
pub fn build(self) -> UpdateProposal {
let mut update_proposal = UpdateProposal::new();
for config_param in self.config_params.iter().cloned() {
update_proposal.changes.push(config_param);
Expand All @@ -59,17 +59,17 @@ impl SignedProposalBuilder {
}
}

pub fn with_proposer_id(&mut self, proposer_id: BftLeaderId) -> &mut Self {
pub fn with_proposer_id(mut self, proposer_id: BftLeaderId) -> Self {
self.proposer_id = Some(proposer_id);
self
}

pub fn with_proposal_update(&mut self, update_proposal: UpdateProposal) -> &mut Self {
pub fn with_proposal_update(mut self, update_proposal: UpdateProposal) -> Self {
self.update_proposal = Some(update_proposal);
self
}

pub fn build(&self) -> SignedUpdateProposal {
pub fn build(self) -> SignedUpdateProposal {
SignedUpdateProposal {
proposal: UpdateProposalWithProposer {
proposal: self.update_proposal.clone().unwrap(),
Expand All @@ -93,17 +93,17 @@ impl UpdateVoteBuilder {
}
}

pub fn with_proposal_id(&mut self, proposal_id: UpdateProposalId) -> &mut Self {
pub fn with_proposal_id(mut self, proposal_id: UpdateProposalId) -> Self {
self.proposal_id = Some(proposal_id);
self
}

pub fn with_voter_id(&mut self, voter_id: BftLeaderId) -> &mut Self {
pub fn with_voter_id(mut self, voter_id: BftLeaderId) -> Self {
self.voter_id = Some(voter_id);
self
}

pub fn build(&self) -> SignedUpdateVote {
pub fn build(self) -> SignedUpdateVote {
let update_vote = UpdateVote {
proposal_id: self.proposal_id.unwrap(),
voter_id: self.voter_id.clone().unwrap(),
Expand Down
2 changes: 1 addition & 1 deletion chain-impl-mockchain/src/testing/e2e/rewards/tax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ pub fn no_tax() {

fn verify_distribute_rewards(
total_reward: u64,
stake_pool_builder: &mut StakePoolDefBuilder,
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, this was clearly wrong...

stake_pool_builder: StakePoolDefBuilder,
expected_stake_pool_reward: u64,
) {
let (mut ledger, controller) = prepare_scenario()
Expand Down
45 changes: 24 additions & 21 deletions chain-impl-mockchain/src/testing/scenario/scenario_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub struct ScenarioBuilder {
config: ConfigBuilder,
initials: Option<Vec<WalletTemplateBuilder>>,
stake_pools: Option<Vec<StakePoolDefBuilder>>,
vote_plans: Vec<VotePlanDefBuilder>,
vote_plans: Option<Vec<VotePlanDefBuilder>>,
}

pub fn prepare_scenario() -> ScenarioBuilder {
Expand All @@ -54,32 +54,32 @@ pub fn prepare_scenario() -> ScenarioBuilder {
config: default_config_builder,
initials: None,
stake_pools: None,
vote_plans: Vec::new(),
vote_plans: None,
}
}

impl ScenarioBuilder {
pub fn with_config(&mut self, config: ConfigBuilder) -> &mut Self {
pub fn with_config(mut self, config: ConfigBuilder) -> Self {
self.config = config;
self
}

pub fn with_initials(&mut self, initials: Vec<&mut WalletTemplateBuilder>) -> &mut Self {
self.initials = Some(initials.iter().map(|x| (**x).clone()).collect());
pub fn with_initials(mut self, initials: Vec<WalletTemplateBuilder>) -> Self {
self.initials = Some(initials);
self
}

pub fn with_vote_plans(&mut self, vote_plans: Vec<&mut VotePlanDefBuilder>) -> &mut Self {
self.vote_plans = vote_plans.iter().map(|x| (**x).clone()).collect();
pub fn with_vote_plans(mut self, vote_plans: Vec<VotePlanDefBuilder>) -> Self {
self.vote_plans = Some(vote_plans);
self
}

pub fn with_stake_pools(&mut self, stake_pools: Vec<&mut StakePoolDefBuilder>) -> &mut Self {
self.stake_pools = Some(stake_pools.iter().map(|x| (**x).clone()).collect());
pub fn with_stake_pools(mut self, stake_pools: Vec<StakePoolDefBuilder>) -> Self {
self.stake_pools = Some(stake_pools);
self
}

pub fn build(&self) -> Result<(TestLedger, Controller), ScenarioBuilderError> {
pub fn build(self) -> Result<(TestLedger, Controller), ScenarioBuilderError> {
if self.initials.is_none() {
return Err(ScenarioBuilderError::UndefinedInitials);
}
Expand Down Expand Up @@ -107,14 +107,17 @@ impl ScenarioBuilder {
let faucets: Vec<AddressDataValue> =
wallets.iter().cloned().map(|x| x.as_account()).collect();

let vote_plan_defs: Vec<VotePlanDef> =
self.vote_plans.iter().map(|x| x.clone().build()).collect();
let vote_plan_fragments: Vec<Fragment> = self
let vote_plan_defs: Vec<VotePlanDef> = self
.vote_plans
.iter()
.map(|builders| builders.into_iter())
.flatten()
.map(|x| x.clone().build())
.collect();
let vote_plan_fragments: Vec<Fragment> = vote_plan_defs
.iter()
.cloned()
.map(|x| {
let vote_plan_def = x.build();
.map(|vote_plan_def| {
let owner = wallets
.iter()
.cloned()
Expand Down Expand Up @@ -211,9 +214,9 @@ impl ScenarioBuilder {
}

fn build_stake_pool(&self, template: StakePoolTemplate) -> StakePool {
let mut builder = StakePoolBuilder::new();
builder.with_owners(template.owners());
builder.with_alias(&template.alias());
let mut builder = StakePoolBuilder::new()
.with_owners(template.owners())
.with_alias(&template.alias());

if let Some(stake_pools) = &self.stake_pools {
let stake_pool_def_opt = stake_pools
Expand All @@ -224,12 +227,12 @@ impl ScenarioBuilder {

if let Some(stake_pool_def) = stake_pool_def_opt {
if let Some(pool_permission) = stake_pool_def.pool_permission() {
builder.with_pool_permissions(pool_permission);
builder = builder.with_pool_permissions(pool_permission);
}
if let Some(tax_type) = stake_pool_def.tax_type {
builder.with_tax_type(tax_type);
builder = builder.with_tax_type(tax_type);
}
builder.with_reward_account(stake_pool_def.has_reward_account);
builder = builder.with_reward_account(stake_pool_def.has_reward_account);
}
}
builder.build()
Expand Down
Loading