Implement multisig example

[richardpringle]

I still need to fix the gas stuff for the mock

• Use repr(packed) with state-keys
• Implement basic multisig example

[iFrostizz]

It's looking really good, short and sweet!

The double spend test is failing and I haven't seen anything preventing it.
The contract call is probably blocked by #1417, it may also make sense to let the actor specify the max_units. If the call fails, it will revert the transaction.
Finally, the Any option from the Voters enum definitely looks interesting but I'm thinking that there should at least be some kind of vesting or locked stake to avoid anyone to just spam the votes. No strong opinion here and we could leave as-is since it's just an example.

[iFrostizz]

It's my fault, I'll need to push on #1417

[iFrostizz]

It's a little weird that the actor check is done after having stored the vote, even though it should not have any security flaw what do you think about having this check before ?

[richardpringle]

I'll have to look again, but I think I fixed this ignore this

[richardpringle]

Honestly, the reason I did it this way is that it's the voter that's paying for the vote, it's really up to them to ensure whether or not they can vote.

The logic flows like this

• if the voter hasn't voted, cast their vote
• if the vote affects quorum, record the change
• if a quorum is reached, execute the proposal

Doing things in this order is much simpler as you only deal with one piece of state at a time. Otherwise, there's more branching logic that I just don't care to optimize for.

[iFrostizz]

Makes sense! It's just a code smell, for example if someone wants to extend the code and expects the votes in the Vote state keys to be part of the voters in order to sum the amount of votes then they might get surprised.

You might as well just write

Suggested change

	if !voters.contains(actor) || votes_remaining == 0 {
	return None;
	}
	let actor = ctx.actor();
	let ProposalMeta {
	voters,
	votes_remaining,
	} = ctx
	.get(ProposalVoteInfo(proposal_id))
	.expect("failed to deserialize")
	.expect("proposal not found");
	if !voters.contains(actor) || votes_remaining == 0 {
	return None;
	}
	ctx.store_by_key(Vote(proposal_id, actor), vote)
	.expect("failed to serialize");
	if !vote {
	return None;
	}

The suggestion diff is not perfect but essentially just moving the "effects" after the "checks", but no changes are required.

[iFrostizz]

🚀 🚀 🚀

[iFrostizz]

Makes sense! It's just a code smell, for example if someone wants to extend the code and expects the votes in the Vote state keys to be part of the voters in order to sum the amount of votes then they might get surprised.

You might as well just write

Suggested change

	if !voters.contains(actor) || votes_remaining == 0 {
	return None;
	}
	let actor = ctx.actor();
	let ProposalMeta {
	voters,
	votes_remaining,
	} = ctx
	.get(ProposalVoteInfo(proposal_id))
	.expect("failed to deserialize")
	.expect("proposal not found");
	if !voters.contains(actor) || votes_remaining == 0 {
	return None;
	}
	ctx.store_by_key(Vote(proposal_id, actor), vote)
	.expect("failed to serialize");
	if !vote {
	return None;
	}

The suggestion diff is not perfect but essentially just moving the "effects" after the "checks", but no changes are required.

[samliok]

can we add some comments to these state variables? Makes it easier for a new dev to come to the examples folder and understand how the multisig example is structured

[samliok]

thinking if there could be some re-entrancy attack here. Could an attacker create two smart contracts, one that creates a proposal and another that votes for it, and then continuously call here?

[samliok]

the gas would run out, but could be a good way to test re-entrancy nonetheless

[richardpringle]

Haha, yeah, I guess it would drain the gas of the quorum voter. In practice, I don't think this is a big deal because you would have to hard-code the vote, which would be an immediate red flag (assuming that you would audit the code that you're voting for).

[samliok]

unit and integration tests are niceeeee

[richardpringle]

@samliok changes since your last review:
3a0adb4

(I'm going to squash them though)