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

Replace ballots with indexed map #114

Merged
merged 11 commits into from
Feb 23, 2022
74 changes: 74 additions & 0 deletions packages/voting-contract/src/ballots.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use cosmwasm_std::{Addr, Storage};
use cw_storage_plus::{Index, IndexList, IndexedMap, MultiIndex};
use tg3::Vote;

use crate::ContractError;

// we cast a ballot with our chosen vote and a given points
#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
pub struct Ballot {
pub voter: Addr,
pub proposal_id: u64,
pub points: u64,
pub vote: Vote,
}

pub struct BallotIndexes<'a> {
// This PrimaryKey allows quering over all proposal ids for given voter address
pub voter: MultiIndex<'a, Addr, Ballot, (u64, Addr)>,
Copy link
Contributor

@maurolacy maurolacy Feb 22, 2022

Choose a reason for hiding this comment

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

That last type parameter is crucial, as it now serves not only for signalling the pk deserialization type, but also and more important, to define the type for typed bounds over the multi-index.

Let's create an issue for updating documentation about it. And, making that parameter mandatory, instead of optional.

Also, if we finish CosmWasm/cw-plus#531, that parameter could be tied to the IndexedMap pk definition directly, or automatically.

Which would be great, and will solve the issues with type-less bounds / ranges once and for all.

Copy link
Contributor

@maurolacy maurolacy Feb 22, 2022

Choose a reason for hiding this comment

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

}

impl<'a> IndexList<Ballot> for BallotIndexes<'a> {
fn get_indexes(&'_ self) -> Box<dyn Iterator<Item = &'_ dyn Index<Ballot>> + '_> {
let v: Vec<&dyn Index<Ballot>> = vec![&self.voter];
Box::new(v.into_iter())
}
}

pub struct Ballots<'a> {
pub ballots: IndexedMap<'a, (u64, &'a Addr), Ballot, BallotIndexes<'a>>,
}

impl<'a> Ballots<'a> {
pub fn new(storage_key: &'a str, release_subkey: &'a str) -> Self {
let indexes = BallotIndexes {
voter: MultiIndex::new(|ballot| ballot.voter.clone(), storage_key, release_subkey),
Copy link
Contributor

@maurolacy maurolacy Feb 22, 2022

Choose a reason for hiding this comment

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

Here, if we go ahead and change the index function signature for MultiIndex, we could instead have

Suggested change
voter: MultiIndex::new(|ballot| ballot.voter.clone(), storage_key, release_subkey),
voter: MultiIndex::new(|(pk, _ballot)| pk.1, storage_key, release_subkey),

and get rid of the need for adding extra fields to the values just for indexing.

Update: See CosmWasm/cw-plus#670.

};
let ballots = IndexedMap::new(storage_key, indexes);

Self { ballots }
}

pub fn create_ballot(
&self,
storage: &mut dyn Storage,
addr: &Addr,
proposal_id: u64,
points: u64,
vote: Vote,
) -> Result<(), ContractError> {
self.ballots.update(
storage,
(proposal_id, addr),
move |ballot| -> Result<_, ContractError> {
match ballot {
Some(_) => Err(ContractError::AlreadyVoted {}),
None => Ok(Ballot {
voter: addr.clone(),
proposal_id,
points,
vote,
}),
}
},
)?;
Ok(())
}
}

pub fn ballots() -> Ballots<'static> {
Ballots::new("ballots", "ballots__proposal_id")
}
69 changes: 22 additions & 47 deletions packages/voting-contract/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
mod ballots;
mod error;
pub mod msg;
#[cfg(test)]
Expand All @@ -7,11 +8,11 @@ pub mod state;
use serde::de::DeserializeOwned;
use serde::Serialize;

use ballots::ballots;
pub use error::ContractError;
use state::{
next_id, proposals, Ballot, Config, Proposal, ProposalListResponse, ProposalResponse,
TextProposalListResponse, Votes, VotingRules, BALLOTS, BALLOTS_BY_VOTER, CONFIG,
TEXT_PROPOSALS,
next_id, proposals, Config, Proposal, ProposalListResponse, ProposalResponse,
TextProposalListResponse, Votes, VotingRules, CONFIG, TEXT_PROPOSALS,
};

use cosmwasm_std::{
Expand Down Expand Up @@ -51,17 +52,6 @@ pub fn instantiate<Q: CustomQuery>(
Ok(Response::default())
}

fn save_ballot(
storage: &mut dyn Storage,
proposal_id: u64,
sender: &Addr,
ballot: Ballot,
) -> Result<(), ContractError> {
BALLOTS.save(storage, (proposal_id, sender), &ballot)?;
BALLOTS_BY_VOTER.save(storage, (sender, proposal_id), &ballot)?;
Ok(())
}

pub fn propose<P, Q: CustomQuery>(
deps: DepsMut<Q>,
env: Env,
Expand Down Expand Up @@ -103,15 +93,7 @@ where
proposals().save(deps.storage, id, &prop)?;

// add the first yes vote from voter
save_ballot(
deps.storage,
id,
&info.sender,
Ballot {
points: vote_power,
vote: Vote::Yes,
},
)?;
ballots().create_ballot(deps.storage, &info.sender, id, vote_power, Vote::Yes)?;

let resp = msg::ProposalCreationResponse { proposal_id: id };

Expand Down Expand Up @@ -148,21 +130,7 @@ where
.was_voting_member(&deps.querier, &info.sender, prop.start_height)?;

// cast vote if no vote previously cast
if BALLOTS
.may_load(deps.storage, (proposal_id, &info.sender))?
.is_some()
{
return Err(ContractError::AlreadyVoted {});
}
save_ballot(
deps.storage,
proposal_id,
&info.sender,
Ballot {
points: vote_power,
vote,
},
)?;
ballots().create_ballot(deps.storage, &info.sender, proposal_id, vote_power, vote)?;

// update vote tally
prop.votes.add_vote(vote, vote_power);
Expand Down Expand Up @@ -362,7 +330,9 @@ pub fn query_vote<Q: CustomQuery>(
voter: String,
) -> StdResult<VoteResponse> {
let voter_addr = deps.api.addr_validate(&voter)?;
let prop = BALLOTS.may_load(deps.storage, (proposal_id, &voter_addr))?;
let prop = ballots()
.ballots
.may_load(deps.storage, (proposal_id, &voter_addr))?;
let vote = prop.map(|b| VoteInfo {
proposal_id,
voter,
Expand All @@ -381,7 +351,8 @@ pub fn list_votes<Q: CustomQuery>(
let addr = maybe_addr(deps.api, start_after)?;
let start = addr.as_ref().map(Bound::exclusive);

let votes: StdResult<Vec<_>> = BALLOTS
let votes: StdResult<Vec<_>> = ballots()
.ballots
.prefix(proposal_id)
.range(deps.storage, start, None, Order::Ascending)
.take(limit)
Expand All @@ -405,18 +376,22 @@ pub fn list_votes_by_voter<Q: CustomQuery>(
start_after: Option<u64>,
limit: usize,
) -> StdResult<VoteListResponse> {
let start = start_after.map(Bound::exclusive);
let voter_addr = deps.api.addr_validate(&voter)?;

let votes: StdResult<Vec<_>> = BALLOTS_BY_VOTER
.prefix(&voter_addr)
// PrimaryKey of that IndexMap is (proposal_id, voter_address) -> (u64, Addr)
Copy link
Contributor

@maurolacy maurolacy Feb 22, 2022

Choose a reason for hiding this comment

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

Also, pk is (the last) part of a (multi-)index key. That's why, after fixing the voter addr with prefix(), you still need to range over the rest of the index key (which matches / corresponds to the pk, in this case).

let start = start_after.map(|m| Bound::exclusive((m, voter_addr.clone())));
Copy link
Contributor

@maurolacy maurolacy Feb 22, 2022

Choose a reason for hiding this comment

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

Another way to do this (just for fun. The current impl is better / clearer):

Suggested change
let start = start_after.map(|m| Bound::exclusive((m, voter_addr.clone())));
let start = start_after.map(|proposal_id| Bound::inclusive((proposal_id+1, Addr::unchecked("")));

An inclusive bound over proposal_id+1 is needed, because we currently don't have something like Addr::MAX (but we could).

This works basically because the (proposal_id, voter) tuple is unique (guaranteed, because it's the pk). So, this will not miss any keys (for any given voter, he could have voted (at most) only once on any given proposal).


let votes: StdResult<Vec<_>> = ballots()
.ballots
.idx
.voter
.prefix(voter_addr)
.range(deps.storage, start, None, Order::Ascending)
Copy link
Contributor

@maurolacy maurolacy Feb 22, 2022

Choose a reason for hiding this comment

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

You can use range_raw here, as you don't use / need the pk. That, or stick with range, and get proposal_id from the (deserialised) pk, below.

That way you can avoid the need for storing the proposal_id as part the value.

I'm now thinking that it would be good that the MultiIndex index function signature, instead of being fn(&T) -> IK, were fn(&PK, &T) -> IK instead. That way, the pk or parts of it can be used for indexing.

That will avoid having to store the voter address for the value as well. Will create an issue for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

That way you can avoid the need for storing the proposal_id as part the value.

So this is already done.

Copy link
Contributor

@maurolacy maurolacy Feb 23, 2022

Choose a reason for hiding this comment

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

Yes. After CosmWasm/cw-plus#670 (if we ever decide to do it), you can avoid the need for storing owner in the value also. As the index function will have that from the pk passed to it, and can take it from there.

This is just an idea at the moment. It will be a breaking change, but I think it may be worth it.

.take(limit)
.map(|item| {
let (proposal_id, ballot) = item?;
let (_, ballot) = item?;
Ok(VoteInfo {
proposal_id,
voter: voter.clone(),
proposal_id: ballot.proposal_id,
voter: ballot.voter.into(),
vote: ballot.vote,
points: ballot.points,
})
Expand Down
13 changes: 1 addition & 12 deletions packages/voting-contract/src/state.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use cosmwasm_std::{Addr, BlockInfo, Decimal, StdResult, Storage, Uint128};
use cosmwasm_std::{BlockInfo, Decimal, StdResult, Storage, Uint128};
use cw_storage_plus::{Item, Map};
use tg3::{Status, Vote};
use tg4::Tg4Contract;
Expand Down Expand Up @@ -253,21 +253,10 @@ fn votes_needed(points: u64, percentage: Decimal) -> u64 {
((applied.u128() + PRECISION_FACTOR - 1) / PRECISION_FACTOR) as u64
}

// we cast a ballot with our chosen vote and a given points
// stored under the key that voted
#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
pub struct Ballot {
pub points: u64,
pub vote: Vote,
}

// unique items
pub const CONFIG: Item<Config> = Item::new("voting_config");
pub const PROPOSAL_COUNT: Item<u64> = Item::new("proposal_count");

// multiple-item map
pub const BALLOTS: Map<(u64, &Addr), Ballot> = Map::new("votes");
pub const BALLOTS_BY_VOTER: Map<(&Addr, u64), Ballot> = Map::new("votes_by_voter");
pub fn proposals<'m, P>() -> Map<'m, u64, Proposal<P>> {
Map::new("proposals")
}
Expand Down