Replace ballots with indexed map #114

ueco-jb · 2022-02-22T10:49:25Z

closes #83

I expanded Ballot structure, so now it contains also address of voter and proposal id.
It got me thinking - now this is basically the same structure as VoteInfo link, BUT... it is using String instead of Addr type as in Ballot now.
Options:

Leave it as it is
Get rid of separate Ballot structure and use VoteInfo in voting-contract everywhere instead
2a) but check for Addr correctness before each query.
2b) replace String with Addr in VoteInfo structure

implementation

…tion from there

maurolacy · 2022-02-22T14:38:41Z

closes #83

I expanded Ballot structure, so now it contains also address of voter and proposal id.

These are in the primary key. I understand you need voter for indexing. proposal_id wouldn't be needed, as it can be extracted from the (deserialized) pk.

It got me thinking - now this is basically the same structure as VoteInfo link, BUT... it is using String instead of Addr type as in Ballot now. Options:

Leave it as it is

Get rid of separate Ballot structure and use VoteInfo in voting-contract everywhere instead
2a) but check for Addr correctness before each query.
2b) replace String with Addr in VoteInfo structure

VoteInfo is perhaps used for responses? I think it makes sense for voter to be String there.

ueco-jb · 2022-02-22T14:44:03Z

proposal_id wouldn't be needed

That's right, I kept it mostly for consistency. Good point, removed.

I think it makes sense for voter to be String there.

Cool, that's why I asked. Anyway, I removed proposal_id so question is irrelevant now.

maurolacy

Looks good to me.

This flagged a number of limitations with the current storage-plus muti-index impl, that we can address in follow-up issues.

maurolacy · 2022-02-22T14:40:46Z

packages/voting-contract/src/lib.rs

+        .ballots
+        .idx
+        .voter
+        .prefix(voter_addr)
        .range(deps.storage, start, None, Order::Ascending)


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.

See CosmWasm/cw-plus#670.

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

So this is already done.

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.

maurolacy · 2022-02-22T14:51:39Z

packages/voting-contract/src/ballots.rs

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


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.

See CosmWasm/cw-plus#669.

maurolacy · 2022-02-22T14:53:51Z

packages/voting-contract/src/ballots.rs

+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),


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.

uint

+1 for pulling that code out into the ballots.rs file. Awesome.

Now that I think about it, shouldn't we mark this as breaking-state?

maurolacy · 2022-02-22T14:59:57Z

packages/voting-contract/src/lib.rs

        .range(deps.storage, start, None, Order::Ascending)
        .take(limit)
        .map(|item| {
-            let (proposal_id, ballot) = item?;
+            let ((proposal_id, _), ballot) = item?;


maurolacy · 2022-02-22T15:04:01Z

packages/voting-contract/src/lib.rs

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


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).

maurolacy · 2022-02-22T15:29:45Z

packages/voting-contract/src/lib.rs

-    let votes: StdResult<Vec<_>> = BALLOTS_BY_VOTER
-        .prefix(&voter_addr)
+    // PrimaryKey of that IndexMap is (proposal_id, voter_address) -> (u64, Addr)
+    let start = start_after.map(|m| Bound::exclusive((m, voter_addr.clone())));


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).

ueco-jb · 2022-02-22T15:34:59Z

@uint

Now that I think about it, shouldn't we mark this as breaking-state?

I actually removed "breaking" label, since all changes are done internally. And although technically state.rs file is changed, it doesn't even need any adaptation in contracts that already uses this new functionality (so tgrade-validator-voting and tgrade-community-pool).

uint · 2022-02-22T16:29:50Z

@uint

Now that I think about it, shouldn't we mark this as breaking-state?

I actually removed "breaking" label, since all changes are done internally. And although technically state.rs file is changed, it doesn't even need any adaptation in contracts that already uses this new functionality (so tgrade-validator-voting and tgrade-community-pool).

Yeah, but if there are live voting contracts out there that have already stored some ballots on the blockchain, they will probably break when migrating since their stored data is structured differently now (Map -> IndexedMap).

ueco-jb self-assigned this Feb 22, 2022

ueco-jb added 9 commits February 22, 2022 14:20

WIP - replace BALLOTS and BALLOTS_BY_VOTER with MultiIndex

ae622d3

implementation

Properly return an error on double vote scenario

e29c213

Move ballots to separate file

a8b9d16

Remove old ballots implementation

fd79002

Make use of new version of Bounds

61cb654

Remove redundant index

4fa71f2

Fix implementation of multiindex pagination

337b953

Bring back queries in voting contract and refer to ballots implementa…

3b4219e

…tion from there

Actually move back all queries back to original places of implementation

0d39caa

ueco-jb force-pushed the 83-replace-ballots-with-indexed-map branch from 330fc50 to e964d5c Compare February 22, 2022 13:20

Add couple comments and remove no longer necessary pub

af211cd

ueco-jb force-pushed the 83-replace-ballots-with-indexed-map branch from e964d5c to af211cd Compare February 22, 2022 13:21

ueco-jb requested review from hashedone, ethanfrey and uint February 22, 2022 13:29

ueco-jb marked this pull request as ready for review February 22, 2022 13:29

ueco-jb requested a review from maurolacy February 22, 2022 13:29

Remove proposal_id field from Ballot structure

165e7f6

maurolacy approved these changes Feb 22, 2022

View reviewed changes

uint approved these changes Feb 22, 2022

View reviewed changes

maurolacy reviewed Feb 22, 2022

View reviewed changes

ueco-jb mentioned this pull request Feb 23, 2022

Release 0.7.0-alpha1 #116

Merged

ueco-jb merged commit 4784f46 into main Feb 23, 2022

ueco-jb deleted the 83-replace-ballots-with-indexed-map branch February 23, 2022 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ballots with indexed map #114

Replace ballots with indexed map #114

ueco-jb commented Feb 22, 2022 •

edited

Loading

maurolacy commented Feb 22, 2022

ueco-jb commented Feb 22, 2022

maurolacy left a comment •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 22, 2022

ueco-jb Feb 23, 2022

maurolacy Feb 23, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

uint left a comment

maurolacy Feb 22, 2022

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

ueco-jb commented Feb 22, 2022 •

edited

Loading

uint commented Feb 22, 2022

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

	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("")));

Replace ballots with indexed map #114

Replace ballots with indexed map #114

Conversation

ueco-jb commented Feb 22, 2022 • edited Loading

maurolacy commented Feb 22, 2022

ueco-jb commented Feb 22, 2022

maurolacy left a comment • edited Loading

Choose a reason for hiding this comment

maurolacy Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

maurolacy Feb 22, 2022

Choose a reason for hiding this comment

ueco-jb Feb 23, 2022

Choose a reason for hiding this comment

maurolacy Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

maurolacy Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

maurolacy Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

maurolacy Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

uint left a comment

Choose a reason for hiding this comment

maurolacy Feb 22, 2022

Choose a reason for hiding this comment

maurolacy Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

maurolacy Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

ueco-jb commented Feb 22, 2022 • edited Loading

uint commented Feb 22, 2022

ueco-jb commented Feb 22, 2022 •

edited

Loading

maurolacy left a comment •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 23, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

maurolacy Feb 22, 2022 •

edited

Loading

ueco-jb commented Feb 22, 2022 •

edited

Loading