eswarasai commented 4 years ago

Description

[x] Added interface for the ease of paying out bounties
[x] Handle different scenarios of paying out bounties i.e., gardener/worker/reviewer
[x] Added reputation only checkbox
[x] Create a multisig transaction with required input parameters to payout bounty

Fixes: #8

eswarasai commented 4 years ago

@troggy - I think this is ready for initial review. Few of the things are missing from below:

Add validation for worker and reviewer address
Not sure if we need to add isRepOnly checkbox and also how it fits in exactly

Let me know your initial review/thoughts, so that I can quickly iterate. Thanks!

troggy commented 4 years ago

thanks, I will review 👍

Not sure if we need to add isRepOnly checkbox and also how it fits in exactly

Yes, it is needed. Sometimes we want just to register reputation, not actual payout. I can imagine that Reputation only checkbox should affect transaction like that:

If "Reputation only" checkbox is unchecked, do nothing
if "Reputation only" checkbox is checked then dapp should add 1 wei to the amount (no need to reflect amount change on the UI of course)

For instance: 200 DAI + "Reputation only" checked → 200 × 10¹⁸ + 1 is sent in transaction 200 DAI + "Reputation only" unchecked → 200 × 10¹⁸ is sent in transaction

eswarasai commented 4 years ago

@troggy - Thanks for the detailed explanation. I'll incorporate the above change as well 👍

One more thing to note is, I'm only using payout() method of the BountyPayout contract as the UI handles the default gardener, worker and reviewer fields. Moreover if the amount of worker/reviewer is 0, the actual payout isn't sent anyway. I hope this behaviour is fine. Thanks!

troggy commented 4 years ago

@eswarasai I've fixed an error in my previous message: In example with 200 reputation payout correct value is

200 × 1000000000000000000 + 1

eswarasai commented 4 years ago

@troggy - No worries. I got the gist of what should be done. I've been going through the leap-contracts PR for Bounty Payout V2 as well for the reference to ensure I'm implementing things right 😅

eswarasai commented 4 years ago

@troggy - Added the reputation only checkbox as well 🙂

johannbarbie commented 4 years ago

filled out this payment:

expected:

function payoutReviewedDelivery() would be called.

actual:

function payout() was called:

troggy commented 4 years ago

@johannbarbie that was me who confused @eswarasai to use payout() for any payment (it is simpler). I wasn't aware of gas savings you get if you use payoutReviewedDelivery() back then

eswarasai commented 4 years ago

@johannbarbie - For the ease of it, I'm just calling the payout method even when there's no worker/reviewer with their respective amounts 0 since the payment won't happen. In case you'd like to have better transaction history, I can do the necessary handling of calling the particular method in case of each scenario.

Just to be sure I don't misunderstand the use cases, please do list which function to be called in each scenario. Thanks 🙂

johannbarbie commented 4 years ago

@johannbarbie that was me who confused @eswarasai to use payout() for any payment (it is simpler). I wasn't aware of gas savings you get if you use payoutReviewedDelivery() back then

because of the colony overhead, it is quite a lot, 500,000 🌻

payout() 1.5M payoutNoReviewer() 1.0M

at 7.5gWei that is 0.6 USD saving in 1 TX !!!

johannbarbie commented 4 years ago

please do list which function to be called in each scenario

gardener + worker + reviewer => payout() gardener + reviewer => payoutReviewedDelivery() gardener + worker => payoutNoReviewer()

eswarasai commented 4 years ago

@johannbarbie - I see that you've already tested the code on mainnet? Apart from the above method change, is there anything else needed to be handled?

I'm wondering if we need the below or not? Let me know if it's of priority.

Add validation for worker and reviewer address

cc: @troggy

troggy commented 4 years ago

Add validation for worker and reviewer address

it should be easy to add. Otherwise, not a priority — the tool is for internal use. IMO

johannbarbie commented 4 years ago

I see that you've already tested the code on mainnet?

yeah, here: https://etherscan.io/tx/0x228b47e236caada562f9d7a31e57ccc98f972d360c741c2ae4716e1febafb100

Apart from the above method change, is there anything else needed to be handled?

in the bountyURL field, there is no length check. if people enter https://github.com/leapdao/MultiSigWallet/issues/8 then only /MultiSigWallet/issues/8 should be converted to hex (everything after leapdao): => 0x2f4d756c746953696757616c6c65742f6973737565732f38 you are doing that already correctly. 👍

the bountyURL field is only 32 bytes in length, so if the part after https://github.com/leapdao is longer than 32 characters, user should see an error. currently i am able to submit longer values, and the tx will fail.

eswarasai commented 4 years ago

@johannbarbie - Thanks for the above comment. I'll add the 32 characters length check as well.

payout() 1.5M payoutNoReviewer() 1.0M

Looking at these values, I'm afraid I need to increase the gas for the transaction options? Right now it's just 500,000. Let me know what should be the gas limits for all the 3 methods. Thanks! 🙂

johannbarbie commented 4 years ago

Looking at these values, I'm afraid I need to increase the gas for the transaction options? Right now it's just 500,000. Let me know what should be the gas limits for all the 3 methods. Thanks! 🙂

thx, don't worry about that 🙂 . the values i've listed are for executing transactions. initializing transactions is way cheaper.

eswarasai commented 4 years ago

@johannbarbie - So based on the below scenarios, I'm assuming that there'll always be either worker or reviewer along with gardener for bounty payout?

gardener + worker + reviewer => payout() gardener + reviewer => payoutReviewedDelivery() gardener + worker => payoutNoReviewer()

In case both worker & reviewer details are not present, would it be alright if I don't allow the Payout to happen? Thanks!

eswarasai commented 4 years ago

@johannbarbie - Updated the PR with all the validations as well as calling respective methods in case of no worker/reviewer. Please do test and let me know if there's anything else pending.

Before merging, if you can let me know the deployed Mainnet address of the Bounty Payout V2 contract, I'll do a quick commit for that as well. Thanks!

cc: @troggy

johannbarbie commented 4 years ago

In case both worker & reviewer details are not present, would it be alright if I don't allow the Payout to happen? Thanks!

in LeapDAO any payouts is supposed to pass by 2 pairs of eyes, hence, in addition to gardener, either worker or reviewer has to be present :)

johannbarbie commented 4 years ago

for max value i saw you used 999999999999999, our biggest bounty is 1400 DAI, so we could use that.

eswarasai commented 4 years ago

@johannbarbie - Thanks for the approval. I can update the max value for amount field to 1400 but I also see the below statement in the policy:

If 2 people work on the bounty together, the payout increases by 1.5x.

Should I increase the max value to 1.5x 1400 = 2100 then?

johannbarbie commented 4 years ago

Should I increase the max value to 1.5x 1400 = 2100 then?

yes, great point 👍

eswarasai commented 4 years ago

@johannbarbie - Updated. Let me know if there's anything else needed or good to be merged 🙂

leapdao / MultiSigWallet

Added button to payout bounty #9

Description

filled out this payment:

expected:

actual: