iov-one / weave

Easy-to-use SDK to build Tendermint ABCI applications
https://weave.iov.one/docs/intro.html
Other
1.12k stars 46 forks source link

CLI tool to upload depositor file with addresses and weights to add to distribution #625

Closed kmw101 closed 5 years ago

kmw101 commented 5 years ago

An authorised user has a CLI tool to upload a csv file with the following columns:

  • Wallet Address
  • Q-Factor (or weight) - decimal between 1 and 0

The file is parsed and errors are displayed to the user. The wallets and q-factor are added to the proposed distribution and is voted on by the governing board.

Revised 05/06/2019

An authorised use has a CLI tool to upload a csv containing the following columns:

The file is parsed and any parsing errors shown to the user.

The output is the creation of a proposal, something like CreateProposalMsg(Batch(SendMsg...) which the economic committee then vote on.

ruseinov commented 5 years ago

Some questions:

  1. Is this supposed to create a new distribution ( or in terms of our app it's going to create a new revenue that would later be distributed) or update an existing one?
  2. In case we are going to create one: it looks like we'll need a list of recipients with weight and address + an Admin address for further updates/modifications.
  3. Weights are currently stored and operated upon as integers, should we keep it that way for simplicity instead of using decimals and converting them?
  4. Currently I don't see any authentication requirements on distribution, so it looks like anyone can create or update one, can't seem to find Admin field being used anywhere except assignment.
kmw101 commented 5 years ago

Some answers:

  1. This updates the existing one. The initial plan was to distribute revenues to validators, this change of requirement has added depositors as additional recipients of the revenues. So we now have revenue going to validators and depositors
  2. As you are updating you don't need to create a new one
  3. You can specify an integer between 0 and 100 as the valid input range. This will be documented as a requirement
  4. Then please drop this requirement as long as it is clear who the proposer is.
ruseinov commented 5 years ago

So since this updates an existing one - we need to implement the Admin logic first I suppose. I haven't worked on distribution extension, but couldn't find any usage.

So basically what we need is to have admin key (that matches the address in revenue obj).

Also bear in mind that we currently distribute funds prior to updating the revenue object. Is that something that still makes sense in this scenario or not? If not - then we'd have to cater for it somehow in weave first.

kmw101 commented 5 years ago

I am not 100% sure of the terminology in the code.

My understanding of the existing code is that the revenue from fees accrues into a revenue account, the governing board sets the q-factor or weighting for each validator and this then applied when the revenue is moved to the distribution account.

The new requirement is for the depositors to be added to the recipients subject to being approved by the governing board. The CLI tool is the way to present the depositors and apply the q-factor or weighting as a proposal for the governing board.

ruseinov commented 5 years ago

I'll explain:

  1. We don't seem to have the auth applied for updating the distribution
  2. We force fund distribution on each update of distribution, before the actual update happens. I suppose this can be bypassed by having the distribution account at 0 balance, as long as this account exists.

Now questions about the process:

  1. After your clarifications my understanding is that CLI tool is not used to directly update the distribution, but rather create a proposal with given weights/recipients?
  2. We specify all the recipients that are included in the distribution, therefore overwriting any existing data? Or rather we are to merge the existing data with the new data? What this means is - we never remove anyone from the distribution, but rather update weights if they already exist there or add new recipients when they don't.
  3. This only happens if the proposal gets approved by Gov
kmw101 commented 5 years ago

To answer your questions

  1. Correct ✔
  2. The recipients only cover the depositors NOT the validators. What would be cleanest would be to clear the distribution list once the distribution has been paid. The Governing Board create a proposal to set the weights for the validator set, once approved the recipients and weights are added to the distribution list. The CLI tool uploads the proposed depositor addresses and weights, the Governing Board then vote to accept, reject, abstain etc. If approved the depositor list is then appended to the distribution list. Note it should not matter in which the Governing board vote.
  3. Correct ✔
ruseinov commented 5 years ago

@ethanfrey I believe that we will need to start with updating the distribution extension for that, let me know if that makes sense:

ethanfrey commented 5 years ago

Current implementation allows resetting the list, but it does the distribution beforehand, if that's okay - we can go with that, but that will require us to send the list of validators and depositors as one at the same time

This is actually good and what any sane service provider would expect (we set the rates BEFORE we provide services, not after). Let's keep it as is.

Since there are no more on-chain checks in any form, we can just use simple SendTx here

Same UI for them, but it prepares a governance proposal, which is basically a BatchMsg with a whole list of SendMsg from the "reward fund" (fee collector) to all the different validators. At whatever rate governance decides.

The utility of distribution was to provide up-front guarantees, which was a good idea from @husio. I think it is correct and we can leave it for when the economists see the light. For now, let's just use SendMsg.

We can brainstorm more tomorrow.

ruseinov commented 5 years ago

Yep, that sounds good. I also like the up-front guarantee model more as otherwise this becomes a mess and also not very blockchain'y.

ethanfrey commented 5 years ago

Yep, that sounds good. I also like the up-front guarantee model more as otherwise this becomes a mess and also not very blockchain'y

Yes, the token economics is becoming very non-blockchainy. :disappointed: Anyway, let's just get this over the line, and wait for external validators to raise the same points, which are more likely to be heard than developers who clearly do not understand incentive mechanisms.

husio commented 5 years ago

After applying https://github.com/iov-one/weave/issues/636 you need to pay 100 IOV in order to distribute funds.

The current implementation distributes funds before the update so that there is a guarantee of the share. This means that someone has to pay 100 IOV in order to update a revenue configuration.

kmw101 commented 5 years ago

The 100 IOV come from the Governing Board fund (address to be supplied).

ethanfrey commented 5 years ago

We will not use the distribution contract as they do not want to provide any guarantees (read: unnecessary restrictions on the off-chain calculations). Just Escrow (hopefully) and Send.

Maybe in the future, the wisdom of the upfront guarantees will be recognized :pray:

husio commented 5 years ago

I think this is achievable with the current version of bnscli. I would like this to be tested by @kmw101 on a real network (antnet is using version 0.16) before we close this issue. This will be also our first user feedback on the bnscli tool. :crossed_fingers:

Here is the example set of commands. Looks like the fee setting is still missing.

$ cat /tmp/issue_625.sh 
#!/bin/sh

set -ex

# Show the current bnscli version to ease the debugging.
bnscli version

if [ ! -f $BNSCLI_PRIV_KEY ]
then
        >&2 echo "Private key not generated. Use 'bnscli keygen' to generate one."
        exit 2
fi

export BNSCLI_TM_ADDR=https://bns.antnet.iov.one:443

source_account="seq:foo/src/1"

# The data can come from a CSV file as well.
(
        bnscli send-tokens -src "$source_account" -dst "seq:foo/dst/1" -amount "1 BTC" -memo "`date` payment"
        bnscli send-tokens -src "$source_account" -dst "seq:foo/dst/2" -amount "666 IOV" -memo "`date` payment"
        bnscli send-tokens -src "$source_account" -dst "seq:foo/dst/3" -amount "999 IOV" -memo "`date` payment"
) \
        | bnscli as-batch \
        | bnscli as-proposal -title "$USER test" -description "$USER testing issue 625" -electionrule 1 \
        | bnscli sign \
        | bnscli submit
kmw101 commented 5 years ago

Happy to coordinate the testing!

husio commented 5 years ago

Once https://github.com/iov-one/weave/pull/746 is merged, with-fee command is available.

I am using antnet for texting this script: export BNSCLI_TM_ADDR=https://bns.antnet.iov.one:443

There are some thing missing in order to successfully run this script.

  1. Source address is rubbish. This does not matter that much, but it would be nice to use an account with funds,
  2. Destination addresses are rubbish but that does not matter,
  3. Fee amount must be set manually and right now it is -amount "1 IOV"
  4. Signature must be done using key that owns an account with funds to pay the above fee.
#!/bin/sh

set -ex

# Show the current bnscli version to ease the debugging.
bnscli version

if [ ! -f $BNSCLI_PRIV_KEY ]
then
        >&2 echo "Private key not generated. Use 'bnscli keygen' to generate one."
        exit 2
fi

source_account="seq:foo/src/1"

# The data can come from a CSV file as well.
(
        bnscli send-tokens -src "$source_account" -dst "seq:foo/dst/1" -amount "1 BTC" -memo "`date` payment"
        bnscli send-tokens -src "$source_account" -dst "seq:foo/dst/2" -amount "666 IOV" -memo "`date` payment"
        bnscli send-tokens -src "$source_account" -dst "seq:foo/dst/3" -amount "999 IOV" -memo "`date` payment"
) \
        | bnscli as-batch \
        | bnscli as-proposal -title "$USER test" -description "$USER testing issue 625" -electionrule 1 \
        | bnscli with-fee -amount "1 IOV" \
        | bnscli sign \
        | bnscli submit
ethanfrey commented 5 years ago

Very nice job.

Note to testers, if you replace the | bnscli submit with | bnscli view you can see what will be submitted. This is also present in all the cli test cases, but sometimes trying code yourself is enlightening

husio commented 5 years ago

Done.