Paytovote Plugin Dev #13

rigelrozanski · 2017-01-27T16:28:22Z

@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

jaekwon · 2017-01-28T20:58:29Z

plugins/paytovote/paytovote.go

+	TypeByteVoteSpoiled byte = 0x03
+)
+
+type P2VPluginState struct {


I think this is a misnomer. It isn't the state of the whole plugin, but one issue. type P2VIssue struct?

aha good eye. P2VIssueState? either/or I agree that this makes more sense than pluginState

jaekwon · 2017-01-28T23:17:45Z

plugins/paytovote/paytovote.go

+	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


We should call these VoteFee and IssueFee. In my mind, "Cost" is what happens to you, a "Fee" is what you pay (and sign).

On another note, why would anyone choose to pay a VoteFee?

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

Merged to develop: tendermint/basecoin@1ff535d

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

jaekwon · 2017-01-28T23:20:08Z

plugins/paytovote/paytovote.go

+		// Save P2VPluginState
+		store.Set(p2v.StateKey(tx.Issue), wire.BinaryBytes(newP2VState))
+
+		returnLeftover(tx.Cost2CreateIssue)


Don't we have to subtract/add Cost2Vote as well?

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

jaekwon · 2017-01-28T23:20:32Z

plugins/paytovote/paytovote.go

+		// Save P2VPluginState
+		store.Set(p2v.StateKey(tx.Issue), wire.BinaryBytes(p2vState))
+
+		returnLeftover(tx.Cost2CreateIssue)


Same thing, doesn't seem like Cost2Vote is not being deducted.

jaekwon · 2017-01-28T23:23:39Z

plugins/paytovote/description.md

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


What's a spoiled vote?

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

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

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

melekes

same comments as Jae, otherwise LGTM

rigelrozanski · 2017-02-02T12:26:51Z

All comments above comment from @jaekwon & @ethanfrey addressed, remaining improvements:

testing
- test the plugin state for vote counts
- test the account balances after transactions
Add a CLI?// maintain consistency with the new basecoin CLI architecture introduced in develop

…ent funds

interim

rigelrozanski · 2017-02-08T21:43:05Z

merged into basecoin-examples

Fix LCD tests dependency

* add token transfer restriction * add multisend validation * revert params test Co-authored-by: secret <[email protected]>

fix: update IAVL version

* feat(staking): add period delegation to genesis * fix(staking): fix tests regarding period delegation * fix(staking): fix test for msg edit validator

jaekwon reviewed Jan 28, 2017

View reviewed changes

melekes approved these changes Jan 29, 2017

View reviewed changes

rigelrozanski changed the base branch from abci_proof to master January 31, 2017 10:24

rigelrozanski changed the base branch from master to develop January 31, 2017 10:24

rigelrozanski force-pushed the paytovote branch from 0fa0db3 to 9337405 Compare January 31, 2017 11:05

ethanfrey and others added 12 commits February 3, 2017 16:06

Add proof support and update dependencies

a4f3d07

Added demo cmd paytovote, to show using plugins in real app

a7c59d9

generated pay2vote app

a4ec104

interim

87278f9

working tests

6d7d9c3

added paytovote description.md

3d86f63

restructure, address all PR comments frey/jae, besides returning unsp…

16974ad

…ent funds

rebase fixes

8c00882

desc. revision

6002282

added return coins

f202191

interim

added CLI, more tests

8ce10e2

app.GetState and fix paytovote test

f033c19

ebuchman force-pushed the paytovote branch from a4d7c1a to f033c19 Compare February 3, 2017 21:30

rigelrozanski closed this Feb 8, 2017

ebuchman deleted the paytovote branch February 10, 2017 21:43

mossid pushed a commit that referenced this pull request Jan 27, 2019

Merge pull request #13 from cosmos/joon/lcd-test-verifier-fix

3e3a252

Fix LCD tests dependency

chengwenxi pushed a commit that referenced this pull request Jan 13, 2021

Add bank transfer restriction (#13)

b4586d2

* add token transfer restriction * add multisend validation * revert params test Co-authored-by: secret <[email protected]>

luchenqun pushed a commit to luchenqun/cosmos-sdk that referenced this pull request Sep 29, 2023

chore: update changelog (cosmos#13)

e45994e

DongLieu pushed a commit to Genuine-labs/cosmos-sdk that referenced this pull request Mar 29, 2024

Merge pull request cosmos#13 from classic-terra/update-iavl

11d6b35

fix: update IAVL version

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paytovote Plugin Dev #13

Paytovote Plugin Dev #13

rigelrozanski commented Jan 27, 2017

jaekwon Jan 28, 2017

rigelrozanski Jan 29, 2017

jaekwon Jan 28, 2017

jaekwon Jan 28, 2017

jaekwon Jan 29, 2017

jaekwon Jan 29, 2017

rigelrozanski Jan 29, 2017

jaekwon Jan 28, 2017

rigelrozanski Jan 29, 2017

jaekwon Jan 28, 2017

jaekwon Jan 28, 2017

rigelrozanski Jan 29, 2017 •

edited

Loading

rigelrozanski Jan 31, 2017

rigelrozanski Jan 31, 2017

melekes left a comment

rigelrozanski commented Feb 2, 2017

rigelrozanski commented Feb 8, 2017

Paytovote Plugin Dev #13

Paytovote Plugin Dev #13

Conversation

rigelrozanski commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melekes left a comment

Choose a reason for hiding this comment

rigelrozanski commented Feb 2, 2017

rigelrozanski commented Feb 8, 2017

rigelrozanski Jan 29, 2017 •

edited

Loading