giveth/liquidpledging

Liquid Pledging

Ethereum contract to delegate donations to projects.

Welcome to the code for the liquidpledging contract, a new way to distribute donated ether while keeping ownership in the hands of the original donor.

Table of content

• Table of content
  • Install
  • Requirements
  • Package
• Run demo
• Help

Install

1. Click Star on this repo near the top-right corner of this web page (if you want to).
2. Join our Riot if you haven't already.
3. Fork this repo by clicking Fork button in top-right corner of this web page. Continue to follow instruction steps from your own liquidpledging repo.
4. The rest of these steps must be done from your machine's command line. Clone your own "liquidpledging" repo:

   git clone https://github.com/GITHUB_USERNAME/liquidpledging.git
   

5. Change directories to liquidpledging:

   cd liquidpledging
   

Requirements

Make sure you have NodeJS (v8.4.0 or higher) and npm (5.4.1 or higher) installed.

You will also need to install embark globally using yarn

npm i -g embark@next

Package

The liquidpledging contract is published as an npm package for developer convenience. To include it as a dependency in your package.json run this from your apps root dirctory.

 npm install liquidpledging --save

Run demo

Follow the instructions on the liquidpleding-demo repo.

Help

Reach out to us on slack for any help or to share ideas.

liquidpledging's Issues

Add Escapable.sol to every contract

I assume we will just have the escapable contract on top of EVERY contract we deploy (because you know people will send silly tokens to any address we create ;-) )

This should be a standard practice

each payment must be confirmed by the vault owner

Currently the vault requires the owner to confirm every payment before it can be withdrawn. Is this the vault we intend to deploy? Or do we want to use a vault with a paymentDelay of say 3 days. Where if the vault owner doesn’t cancel the payment, the recipient can withdraw after the time period w/o direct interaction from the vault owner

Liquid Pledging has issues when there are a lot of small pledges with from different owners

there is that pesky block gas limit... we need to optimize and maybe even find a way to consolidate pledges before they payout

LiquidPledgingBase: getProjectLevel() double check

I am 80% sure this works, but i would like to confirm there are tests that prove it works as expected for the case that there is one parent project (that it returns 2)

it might need to be return getProjectLevel(parentNM+1);?

Just a check, I expect I am wrong as I am just a muggle playing around in a world of wizards.. but I'd hate to pull a Gun Sirir and see a potential bug but not the devs.

    function getProjectLevel(PledgeAdmin m) internal returns(uint) {
        assert(m.adminType == PledgeAdminType.Project);
        if (m.parentProject == 0) return(1);
        PledgeAdmin storage parentNM = findAdmin(m.parentProject);
        return getProjectLevel(parentNM);

I want to learn how to write tests one day.......... one day.....

We need error messages for revert(), require(), and assert()

The entire contract needs to utilize solidity's new functionality. This might be a fun way to let community members contribute to the project.

Each error message should be it's own PR.

See https://media.consensys.net/when-to-use-revert-assert-and-require-in-solidity-61fb2c0e5a57 for more background info

liquidPledgingBase.sol Name Changes

pm = cm (after the change to campaign)

transfer = transferPledge (transfer is a solidity common name, not easy to read or audit) this is global name change and a tough one to do...

findNote = findOrCreatePledge (why 2 functions with same name? not easy to read or audit)

possible for plugins to prevent transfer of eth

In the current plugin system, if present, we call the plugins for the pledge owner, all delegates in the chain, and the intendedProject. This presents a few issues.

1. it is possible for any malicious actor to write a plugin that either throws or returns 0 when called and thus makes it impossible to transfer the pledge, essentially causing the eth to become stuck in the pledge for eternity.

2. it is possible for an intendedProject to prevent the owner of the pledge from rejecting the delegation. The project plugin can easily detect if they are being rejected by looking for fromPledge.intendedProject == projectPlugin.idProject && toPledge.owner != projectPlugin.idProject && toPledge.intendedProject != projectPlugin.idProject

One possible solution is to have a whitelist consisting of a list of approved plugin bytecode hashes. Using assembly, we can verify the bytecode at the provided plugin address in the initial addProject/Delegate/Giver call and throw if the provided plugin isn't an approved plugin.

change url to ipfsHash

talked with @GriffGreen and decided to use ipfsHash instead of url

change url -> ipfsHash

Global Name Changes

I would like to propose many name changes:

Donor = Giver

Project = Campaign

Note = Pledge

NoteManager = PledgeAdmin

Manager = Admin

hNote2ddx = hNote2idx

NotPaid = Pledged

proposedProject = intendedCampaign

vault should transfer eth after calling liquidPledging.confirmPayment

PR #24

add return idManager in `liquidPledgingBase`

just a reminder

should not be able to donate to a canceledProject

If a project is canceled, it can still receive transfers.

Bug when vetoing a delegation

To veto a delegation, I call the the transfer method with the delegate as the recipient, thus removing the proposedProject from the note.

However when we do this, we call appendDelegate, and append the recipient to the delegationChain, even if the recipient is already last in the delegation chain.

Thus with a picky donor who rejects often or whos note has been passed around a lot, it is possible to have n.delegationChain.length > MAX_DELEGATES which will throw and the donor will have no way to reject the delegation.

function appendDelegate(uint64 idNote, uint amount, uint64 idReceiver) internal  {
        Note storage n= findNote(idNote);

        require(n.delegationChain.length < MAX_DELEGATES); //TODO change to revert and say the error
        uint64[] memory newDelegationChain = new uint64[](n.delegationChain.length + 1);
        for (uint i=0; i<n.delegationChain.length; i++) {
            newDelegationChain[i] = n.delegationChain[i];
        }
...

We either need a separate method rejectProposedProject or to change appendDelegate to not append the idReceiver if they are already the last delegate in the chain.

something like...

function appendDelegate(uint64 idNote, uint amount, uint64 idReceiver) internal  {
        Note storage n= findNote(idNote);

        if (n.delegationChain[ n.delegationChain.length ] == idReceiver) {
            // the last delegate in the chain is already the idReceiver
            uint64[] memory newDelegationChain = n.delegationChain;
        } else {
            require(n.delegationChain.length < MAX_DELEGATES); //TODO change to revert and say the error
            uint64[] memory newDelegationChain = new uint64[](n.delegationChain.length + 1);
            for (uint i=0; i<n.delegationChain.length; i++) {
                newDelegationChain[i] = n.delegationChain[i];
            }

            // Make the last item in the array the idReceiver
            newDelegationChain[n.delegationChain.length] = idReceiver;
        }

        uint64 toNote = findNote(
                n.owner,
                newDelegationChain,
                0,
                0,
                n.oldNote,
                PaymentState.NotPaid);
        doTransfer(idNote, toNote, amount);
    }

Consider design changes to the vault to make it more modular

I like the style of vault that dapphub uses: https://github.com/dapphub/ds-vault

They have a system of authorization checks using https://github.com/nexusdev/ds-auth

Their code is poorly commented and difficult to read, but you get the idea. With a series of external calls to short trusted contracts you may be able to better formally verify the contract down the line and it allows for modularity as some people may want some checks and some people may not want it, those decisions can be made when deploying the contract...

The biggest problem is that this creates alot of DELEGATECALLs which costs a lot of gas :-P so it may not be worth it....

We need a standard deployable donate contract

To make donations easy, we need a simple contract that can be people's donation address. This contract can call donate() in LP and collect the silly tokens people will send and such.

This is similar to the fund forwarder, it should also make sure the tokens that will inevitably get sent to these contracts do not get lost in the shuffle.
https://github.com/Giveth/fund-forwarder/blob/master/contracts/FundForwarder.sol

Redundant getter in LiquidPledgingMock

There's a getter in LiquidPledgingMock.sol for mock_time. It seems like this is redundant since it's already public access. This and I would switch mock_time to mixed case. I also think it might make sense to be able to set the block time for the mock in the constructor.

Use the contracts in the common-contract-deps repo Specifically Owned.sol

https://github.com/Giveth/common-contract-deps/tree/master/contracts

Vault.sol Name Changes

PaymentState = PaymentStatus

Payment[] = PaymentList[]

ref = idPledge

We need better comments

they should follow natspec and be consistant with the comments in the LiquidPledgingBase.

Renaming LiquidPledgingBase: idxDelegate vs idDelegate & projectID

1. projectId --> idProject
   projectId is listed just a couple times where as idProject is listed a bunch, please change!

2. idxDelegate vs idDelegate

This is sooooo confusing, and I am fairly certain it is used inconsistently throughout LiquidPledging base

If we follow the pattern laid out with idProject and idGiver, idDelegate should refer to the id number in the admins array... but we call that idxDelegate in addDelegate(), updateDelegate(), getDelegateIdx() (which has a messed up name cause it has the idxDelegate as a parameter!)

Some times idxDelegate is used the way i would expect it to be used, as the index number for its authority in the delegation chain... this happens in getPledgeDelegate() and in the function namegetDelegateIdx();-)

I would love to hear comments on this, but i see 2 proposals:

My preferred (and easier):
Check all the smart contracts and tests for consistent use of:
idDelegate: the id number in the admin array
idxDelegate: the index number in delegateChain array, different for each specific Pledge

Option 2:

Check all the smart contracts and tests for consistent use of:
idDelegate: the index number in delegateChain array, different for each specific Pledge
idxDelegate: the id number in the admin array

And change:
idGiver -> idxGiver
idProject -> idxProject

add url field to noteManager

It's a good idea to have a link on the blockchain for delegates and campaigns... it prob isnt needed for donors, but still nice to have :-D

delegationChain not maintained when vetoing delegation

If the delegationChain contains delegate1 & delegate2 and the pledge owner veto's the intendedProject by transfering back to delegate2, the delegationChain is reset and only delegate2 exists in the chain.

test exists at:
https://github.com/Giveth/liquidpledging/blob/veto_delegation/test/DelegationChain.js#L152