Paytovote Plugin Dev #13
Conversation
plugins/paytovote/paytovote.go
Outdated
| TypeByteVoteSpoiled byte = 0x03 | ||
| ) | ||
|
|
||
| type P2VPluginState struct { |
There was a problem hiding this comment.
I think this is a misnomer. It isn't the state of the whole plugin, but one issue. type P2VIssue struct?
There was a problem hiding this comment.
aha good eye. P2VIssueState? either/or I agree that this makes more sense than pluginState
plugins/paytovote/paytovote.go
Outdated
| Issue string //Issue being voted for | ||
| ActionTypeByte byte //How is the vote being cast | ||
| Cost2Vote types.Coins //Cost to vote | ||
| Cost2CreateIssue types.Coins //Cost to create a new issue |
There was a problem hiding this comment.
We should call these VoteFee and IssueFee. In my mind, "Cost" is what happens to you, a "Fee" is what you pay (and sign).
There was a problem hiding this comment.
On another note, why would anyone choose to pay a VoteFee?
There was a problem hiding this comment.
Oh maybe this is because Counter includes Cost in the Tx. I added that just to test things, so I can see how it's misleading. We should remove Cost from Counter too, make it a fee, and set the cost upon counter.New(name, cost).
There was a problem hiding this comment.
Merged to develop: tendermint/basecoin@1ff535d
There was a problem hiding this comment.
Yes, that was the inspiration for using cost. Will replace with fee. Also, I think it makes sense to include a voting "fee" (hence the name pay to vote) but I've been conceptualizing it like you are paying an assigned vote token and not a regular currency per-se. Along these lines within the tests, the fees are of the types "votingTokens" and "issueTokens" (token allowing you to create an issue). Let me know your thoughts, I'm open to suggestions
plugins/paytovote/paytovote.go
Outdated
| // Save P2VPluginState | ||
| store.Set(p2v.StateKey(tx.Issue), wire.BinaryBytes(newP2VState)) | ||
|
|
||
| returnLeftover(tx.Cost2CreateIssue) |
There was a problem hiding this comment.
Don't we have to subtract/add Cost2Vote as well?
There was a problem hiding this comment.
Yes, I have this setup to return within the returnLeftover function variable (which needs to be completed, this is a remaining issue) Maybe renaming returnLeftover to something less ambiguous that implies that it's subtracting cost resolves this?https://github.com/tendermint/basecoin/blob/bfac5437c34dc513757c56c048531f04ea3acf60/plugins/paytovote/paytovote.go#L119
plugins/paytovote/paytovote.go
Outdated
| // Save P2VPluginState | ||
| store.Set(p2v.StateKey(tx.Issue), wire.BinaryBytes(p2vState)) | ||
|
|
||
| returnLeftover(tx.Cost2CreateIssue) |
There was a problem hiding this comment.
Same thing, doesn't seem like Cost2Vote is not being deducted.
plugins/paytovote/description.md
Outdated
| - Create a non-existent issue | ||
| - submit a vote for an existing issue | ||
| - submit a vote against an existing issue | ||
| - submit a spoiled vote to an existing issue |
There was a problem hiding this comment.
It can be important symbolic gesture in a democratic system, depending on the laws it could even force a revote. https://en.wikipedia.org/wiki/Spoilt_vote#Intentional_spoiling http://wiki.c2.com/?SpoiledBallot
There was a problem hiding this comment.
I think I'll remove this to avoid confusion, both Jae and Frey confused. Future iterations of this app can include more voting options than vote for and against, but that's enough for this demo app
There was a problem hiding this comment.
It may make sense to have the votes and the type of vote be arrays to allow for a wide selection of vote types (such as voting between a long list of candidates). On this train of thought it would be cool to allow different voting styles such as ranked voting systems (https://plato.stanford.edu/entries/voting-methods/)
rigelrozanski
force-pushed
the
paytovote
branch
from
0fa0db3
to
9337405
Compare
|
All comments above comment from @jaekwon & @ethanfrey addressed, remaining improvements:
|
interim
|
merged into basecoin-examples |
* add token transfer restriction * add multisend validation * revert params test Co-authored-by: secret <secret@bianjie.ai>
fix: update IAVL version
@ethanfrey earlier comments:
Thanks for getting a demo up. I have a number questions on the plugin, maybe you can clarify them in the docs or update the code, then I will merge it in.
Mainly design/docs:
Why do we have a Valid flag, which is set by the user and no purpose except to raise an error if not set?
What are spoiled votes? Are they important in the design?
You can register the plugin multiple times under different names, each instance can support votes for multiple issues. There should be one way (not two) to enable this multiplicity, counter plugin has one count per plugin, so it makes sense to allow multiple plugins. Or if there is a reason for this document it, please.
Every tx decides the required cost? This is odd. I would believe that the Plugin is initialized with the cost to create an issue, which is then demanded of every CreateIssue transaction. And upon creating an issue, I specify the cost for each vote, which is stored in the P2VPluginState. Right now, every voter can choose how much to pay, and each is counted equally.
Code details:
It would be nice to finish the TODO on line 121 (returning unspent coins) before merging. Also, returning them on failed transactions, so errors are not so expensive
I think it would make sense to have two different Tx types, TxCreateIssue, which specifies Issue and Cost2Vote, and TxVote, which specifies Issue and Vote (enum - yes, no, spoil). You can manually prepend a byte to determine wich type of structure it is, or use go-wire's magic to do this. Here is an example: https://github.com/ethanfrey/signedpost/blob/master/txn/transactions.go
If we provide sample code to help people build their own apps, it should demonstrate best practices as far as we can.
Please ping me again when you address these issues (through code or docs), and I will review again and merge