ubiquity / ubiquibot

Putting the 'A' in 'DAO'
https://github.com/marketplace/ubiquibot
MIT License
17 stars 61 forks source link

Gnosis Safe Interaction Module #13

Open 0x4007 opened 2 years ago

0x4007 commented 2 years ago

We only officially set up bounties on the ubiquity-dollar repo but all the same rules we will try and apply here.

The purpose of this module is to allow the Bounty Bot to be able to automatically pay out (or request to pay out) bounties via our Bounties Multisig

However, I realize that we should support multiple chains because when gas fees are high again, I think using a rollup like Optimism or a sidechain like Gnosis Chain (formerly known as xDAI Chain) or even a rollup on a sidechain (Arbitrum on Gnosis Chain 'AoX' kek) could be quite useful.

This should be implemented as a TypeScript module which can be imported and invoked from the core bot logic easily.

The main function parameters should look something like as follows:

// defaults in comments
interface Params {
    network?: number; // 1
    from: string; // this should be the Gnosis Safe address on the network
    amount: string;
    token: string;
    to: string;
    execute?: boolean; // false
    gwei?: number; // auto
}
  1. The network ID (Should test for support on Mainnet, Optimism, Gnosis Chain)
  2. The "from" Gnosis Safe address
  3. The amount of tokens : string (or whatever datatype Ethers BigNumber uses I guess)
  4. The token address : string
  5. The destination address : string
  6. "execute transaction" : boolean
  7. Gas speed override : number (normally this should be automatically determined, prioritizing speedy transfers. An override seems like a useful capability)

I'm not sure at the moment of a useful return value. Ideally we should be able to track the transactions for accounting purposes though.

Environment variables should include an Ethereum private key for a dedicated EOA for the UbiquiBot.

Lastly I would recommend being mindful on limiting the use of dependencies so that we can use @vercel/ncc and host all of the compiled code on GitHub Actions.

Could be useful https://www.npmjs.com/package/@gnosis.pm/safe-core-sdk

3commascapital commented 2 years ago

happy to take a look at this as soon as i am done with cspell

0x4007 commented 1 year ago

Before we saddle up on this bounty price, we need to figure out a viable strategy in order to be able to handle accounting.

I'm not sure at the moment of a useful return value. Ideally we should be able to track the transactions for accounting purposes though.

It is important for us to be able to audit the payments far in the future, ideally keeping track of every transaction hash and the dollar value of each transaction hash (I think we should record the dollar amount in the database instead of trying to read the chain and calculating it - seems significantly easier this way)

For some extra context, in the SQL database in my current planned architecture I plan to tally up total_owed_amount per bounty hunter, and then have another column called total_paid_amount. This means that if total_owed_amount - total_paid_amount = 0 then we have no outstanding payments to the bounty hunter.

We probably will need to create a new table in the database as well to log all the payments.

3commascapital commented 1 year ago

One will need the private key of the signing exposed to the code (at a minimum) via env var or otherwise for this functionality. I am not sure if special attention is required from anyone on the team there.

0x4007 commented 1 year ago

One will need the private key of the signing exposed to the code (at a minimum) via env var or otherwise for this functionality. I am not sure if special attention is required from anyone on the team there.

Encrypted secrets is your answer its not a big deal.

https://docs.github.com/en/actions/security-guides/encrypted-secrets

If it comes down to it, we can limit the GitHub Action signer's authority on the smart contract level. Gnosis offers signer control modules.

3commascapital commented 1 year ago

well, it's more of a question of key management - seed vs pk use. it simply isn't setup to take private keys currently. with a few updates it can be

3commascapital commented 1 year ago

as in, if you want to have a master seed and have the first 2 accounts be your safe owners, then you can do that and only give the bot one of the addresses to make account management easier. you would only have to expose one of the account private keys, but the seed could have access to both. however you wish to resolve that relationship

0x4007 commented 1 year ago

We just need a single private key for the bot. This is far more convenient if/when exporting/importing from another wallet e.g. MetaMask. The access control is handled on the Safe side.

To clarify, the mnemonic isn't useful because if the private key or mnemonic are compromised then we obviously need to rotate them. However, dealing directly with private keys only makes it far more convenient when the operator (aka me or treasurers of partner projects) want to be able to manually invoke the signer via their MetaMask etc.

0x4007 commented 1 year ago

Didn't realize that BigInt is natively supported - we should just use that

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

0xcodercrane commented 1 year ago

oh nice bot!

0xcodercrane commented 1 year ago

I am gonna make things more clear based on current architecture.

Discussed in https://github.com/ubiquity/bounty-bot/discussions/1

Originally posted by **pavlovcik** September 10, 2022 - [ ] Paying Bounties - This is https://github.com/ubiquity/bounty-bot/issues/13 to allow for automatic payments OR queuing payments via Gnosis Safe. - This should check if the Bounty Hunter has a registered address in the database. If they don't the Bounty Bot should request for their address and then request payment from the bounty masters (could be simpler logic to implement this way; otherwise its best for the bot to handle collecting the payment address and then also automatically making the payment and posting the transaction hash) - Should also check who recruited the Bounty Hunter and give them their commission. ### How to resolve Handling payments should get started upon successful confirmation about hunter's registered wallet address. It should get the amount and payment token address (basically USDC as of now) from price label. Both checks should happen here. - 1/ There are two types of price labels, single labels (100 usdc, 200 usdc, ...) and range labels (100-1000 usdc, 200-2000 usdc...). In case of range price label, it should send confirmation request to bounty masters internally or externally. - 2/ Safe wallet balance needs to be checked and should send warning to @ubiquity/bounty-masters internally or externally. Once both checks get passed, it will create a payment request using [safe-core-sdk](https://www.npmjs.com/package/@safe-global/safe-core-sdk) and send the approval request to @ubiquity/bounty-masters. If the payment tx has mined on the chain, txhash should be recorded in the database and posted on the issue with tagging the assigned hunter. cc: @pavlovcik @3commascapital
0x4007 commented 1 year ago
  • it should send confirmation request to bounty masters internally or externally.
  • Ideally the "user interface" (user input/output) for the bot should be entirely through comments on GitHub.
    • In the future I see a possibility to extend the UI (inputs and outputs) by injecting data with a chrome extension (e.g. displaying a bounty hunter's level next to their username.
  • However you bring up a great edge-case with a bounty price range and the bot thinking that its time to pay.

We really should have set a final price before the bounty hunter has finished, so thats the fault of the bounty masters if this happens. The simple answer honestly would be that the bot posts a message stating that we must set a price (via label) before the payment can be processed.

3commascapital commented 1 year ago

how should i rebase and push now that the option is not available in this pr?

0x4007 commented 1 year ago

You can always open a new PR (and associate with this issue.)

0x4007 commented 1 year ago

Hey so I just found a project on Twitter that basically is the exact same feature we are working on. I highly recommend you guys take a look at their implementation.

https://twitter.com/jj_ranalli/status/1595169970228498447?s=46&t=qcOXMvSMNk5-puL1oVlJfQ

https://github.com/slice-so/merge-to-earn

https://github.com/slice-so/merge-to-earn/pull/4

0xcodercrane commented 1 year ago

that's a good example @3commascapital .

3commascapital commented 1 year ago

yeah, i like that they use shares. would be easy enough to create a contract to account for distributions across many people without transferring to them. let the funds accumulate until those contributors wish to withdraw

0x4007 commented 1 year ago

It seems like no activity has occurred on this bounty for awhile so I'm releasing it back to the DevPool. @0xcodercrane can you confirm the status?

0xcodercrane commented 1 year ago

hmm yes I agree to release back to the DevPool

rndquu commented 1 year ago

We could use dai's permit()

On our side we:

  1. Store bot's private key in github secrets
  2. When bounty hunter asks for payment the bot checks a few conditions (PR author, whether payment hasn't already been paid, etc...) and posts permit params

So we could simply keep some DAI on the bot's address. We also don't need to pay gas fees.

Example:

  1. Bounty hunter assigns a bounty to himself via /assign
  2. Bounty hunter solves the bounty and PR is merged
  3. Bounty hunter registers his wallet address via /wallet 0x01
  4. Bounty hunter asks for bounty via /payout
  5. Bot responds with something like:
    {
    owner: 0x01, // bot address
    spender: 0x02, // bounty hunter's address
    value: 1000000000,
    deadline: 16723948783,
    v: 240,
    r: 0x832482374823,
    s: 0x837483748374
    }

This way only bounty hunter can use this permit params. Then bounty hunter calls permit() and transferFrom() and gets his payment.

0x4007 commented 1 year ago

I think we should generalize it with some type of approve(); on the Gnosis Safe so that we can have it work with our Ubiquity Dollars.

I think its a but unusual for the Bounty Hunter to write /payout on a GitHub comment, but given that this is where all of our commands are currently interfacing, and because I don't have a better suggestion; lets proceed with /payout.

rndquu commented 1 year ago

so that we can have it work with our Ubiquity Dollars

Our ubiquity dollar is also an ERC20 token so it also has the permit() method. Just send some UAD to a bot address, store private key in github actions and the bot will post permit signatures exactly for UAD. Right now UAD is not actively used so it makes sense to start with DAI and switch to UAD later.

we should generalize it with some type of approve(); on the Gnosis Safe

I can't understand why we need the Gnosis Safe contract. Gnosis Safe is a multisig wallet which means there should be multiple parties to approve transaction.

Consider an example:

  1. We're using Gnosis Safe which needs 2 out of 2 confirmations to process transaction (bot's private key and admin's private key)
  2. Bounty hunter solves PR and posts /payout
  3. The bot posts offchain permit
  4. Admin posts (i.e. approves) his offchain permit
  5. Bounty hunter executes the payout transaction with those 2 permits

In the flow above there is no need for the bot's offchain permit because in the end admin has to approve the payout by posting his signature. So if we remove the bot's permit (step 3) then we are left only with admin's permit. Using Gnosis Safe for 1 wallet seems to be an overkill. So we can put the Gnosis Safe aside and simply use DAI's permit or UAD's permit.

0x4007 commented 1 year ago

I see okay. The two use cases I want to accomodate are:

  1. The partner project wants to manually approve of payouts.
  2. The partner projects wants payouts to happen automatically.

If the partner can configure that in their repo's bot settings, and we can accomodate both scenarios without a Gnosis Safe, then lets proceed with your suggestion!

One last consideration is that we currently do have a Gnosis Safe for other team members @sergfeldman @Draeieg can payout bounties if I'm not around etc. It's nice to also support this, but we use it infrequently so all manual is probably ok.

0x4007 commented 1 year ago

Sorry, just getting acclimated to the new priority multiplier. Looks like it doesn't have the updated label names. It should be Priority: 1 (Medium)