Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Multi Token Rewards #136

Closed
Tracked by #753
rndquu opened this issue Sep 14, 2023 · 19 comments
Closed
Tracked by #753

Support Multi Token Rewards #136

rndquu opened this issue Sep 14, 2023 · 19 comments

Comments

@rndquu
Copy link
Member

rndquu commented Sep 14, 2023

Depends on ubiquity/ubiquibot#769

Original meta issue

Right now when a bounty hunter opens a permit URL (example) then he is able to redeem only a single token reward. When multiple token rewards feature is implemented then the permit URL query will contain a base64 encoded array of permits which should allow bounty hunters to redeem multiple token rewards for a single issue.

What should be done:

  1. Parse permit URL query param and display a list of claimable rewards. This should work with any amount of permits being passed in.
  2. Add a "Claim" button for each reward row so that bounty hunter could redeem the reward

P.S. Don't forget to refactor the "invalidate nonce" button (which should be displayed for each reward row only for permit owner)

@0x4007
Copy link
Member

0x4007 commented Sep 14, 2023

I guess it would make sense at this point to also specify that it should support an arbitrary array length of permits.

Then, technically, we can easily make a tool to generate a new payment link with all of the outstanding (unclaimed) permits for the contributor.

@rndquu
Copy link
Member Author

rndquu commented Sep 14, 2023

I guess it would make sense at this point to also specify that it should support an arbitrary array length of permits.

Yes, I actually meant that this issue should support claiming an array of permits

Then, technically, we can easily make a tool to generate a new payment link with all of the outstanding (unclaimed) permits for the contributor.

I didn't get it. This issue implies that ubiquity/ubiquibot#769 will refactor the permit URL to contain a base64 encoded array of permits for different reward tokens. So I don't really understand what is the "new payment link" you're referring to.

@0x4007
Copy link
Member

0x4007 commented Sep 14, 2023

The original vision was to enable support for the following scenario:

  1. The contributor completes a task for Ubiquity. They are now eligible for 100 WXDAI and 10 UBQ as a reward.

However I expanded the vision to accomodate the following continuation of the scenario:

  1. Meanwhile the contributor continues adding value by adding comments across multiple other open issues.
  2. The contributor now has several unclaimed permits scattered across multiple issues.

Until we complete the "permit rollup" idea (leveraging the database for accounting, and only generating permits when the contributor wants to cash out) it could be interesting to build a simple tool that will aggregate all of these unclaimed permits and combine them into a single link for the contributor to conveniently be able to claim everything.

This could be either inside of the pay.ubq.fi dApp directly, a bot command, or somewhere else. I have a feeling it would be most elegant from inside of the dApp though.

@rndquu
Copy link
Member Author

rndquu commented Sep 15, 2023

The original vision was to enable support for the following scenario:

  1. The contributor completes a task for Ubiquity. They are now eligible for 100 WXDAI and 10 UBQ as a reward.

However I expanded the vision to accomodate the following continuation of the scenario:

  1. Meanwhile the contributor continues adding value by adding comments across multiple other open issues.
  2. The contributor now has several unclaimed permits scattered across multiple issues.

Until we complete the "permit rollup" idea (leveraging the database for accounting, and only generating permits when the contributor wants to cash out) it could be interesting to build a simple tool that will aggregate all of these unclaimed permits and combine them into a single link for the contributor to conveniently be able to claim everything.

This could be either inside of the pay.ubq.fi dApp directly, a bot command, or somewhere else. I have a feeling it would be most elegant from inside of the dApp though.

The audit page does pretty much the same thing. We could add a new "Combine permits" button there. So for a bounty hunter the flow would be this one:

  1. Bounty hunter solves multiple issues
  2. Bounty hunter runs the audit script across the repos where he solved issues
  3. Bounty hunter clicks the "Combine permits" button
  4. All bounty hunter's unclaimed permits are combined in a single permit URL and shown to the bounty hunter
  5. Bounty hunter clicks on the "combined permits" URLs and pay.ubq.fi is opened with a long list of unclaimed permits

@0x4007
Copy link
Member

0x4007 commented Sep 23, 2023

The audit page does pretty much the same thing. We could add a new "Combine permits" button there. So for a bounty hunter the flow would be this one:

It doesn't make sense from a user experience perspective. To clarify: the audit tool is intended for admins and billing_managers to audit cash outflows. We should import that logic into the pay.ubq.fi rewards interface because that is intended for contributors to use for receiving payments.

Perhaps it makes sense to import that logic into the Rewards dApp from the Audit dApp?

@rndquu
Copy link
Member Author

rndquu commented Sep 26, 2023

This all sticks to github API rate limits.

We can't use the bot's installation token for all of our dapps because we would quickly reach github's API rate limits.

The Audit dApp uses github PAT provided by an end user. So the user must manually generate a new github PAT, paste it in the "GITHUB PERSONAL ACCESS TOKEN" text field and run the audit script. Overall this is not a great UX.

For all of the future dapps (Rewards dApp, Combine Permits dApp, etc...) we should find a more user friendly way to get users' github access tokens.

Github apps support generating user-based tokens. This brings us 2 options (regarding the "combine permit URLs" feature):

  1. Use pay.ubq.fi + github web flow auth
  2. Use a new bot's command + github device flow auth

Option 1: pay.ubq.fi + github web flow auth

How it works from a bounty hunter's view:

  1. Bounty hunter opens (for example) pay.ubq.fi/combine-rewards
  2. Bounty hunter clicks on "Login with Github"
  3. Bounty hunter is redirected to github.com where he authorizes the bot's app
  4. Bounty hunter is redirected back to pay.ubq.fi/combine-rewards
  5. Now we have user's github token which we can use for API calls

How it works under the hood:

  1. Bounty hunter opens (for example) pay.ubq.fi/combine-rewards
  2. Bounty hunter clicks on "Login with Github"
  3. Bounty hunter is redirected to https://github.com/login/oauth/authorize?client_id=GITHUB_APP_CLIENT_ID where he authorizes the bot's app
  4. Bounty hunter is redirected to the auth callback URL (example: pay.ubq.fi/combine-rewards?code=AUTH_CODE)
  5. Backend server calls POST https://github.com/login/oauth/access_token and exchanges the code GET param (from step 4) for the user's access token
  6. Now we have user's github token which we can use for API calls

This flow requires the backend server. Right now pay.ubq.fi is a plain frontend app so we will need to refactor it to have a backend.

Option 2: New bot's command + github device flow auth

How it works from a bounty hunter's view:

  1. Bounty hunter posts a new command /combine-rewards
  2. The bot replies with Open https://github.com/login/device and enter the code ABC-123
  3. Bounty hunter opens https://github.com/login/device and enters the code
  4. After some time the bot gets user's access token

How it works under the hood:

  1. Bounty hunter posts a new command /combine-rewards
  2. The bot calls POST https://github.com/login/device/code and gets the auth code (example: ABC-123)
  3. The bot replies with Open https://github.com/login/device and enter the code ABC-123
  4. The bot starts polling POST https://github.com/login/oauth/access_token until user enters the auth code
  5. Bounty hunter opens https://github.com/login/device and enters the code
  6. The poll (from step 4) gets 200 OK with user's access token

Right now the bot is deployed on netlify functions which have a timeout of 10 seconds. Chances are that a user will not be on time with entering the auth code until the netlify's timeout so this flow seems to be not a good fit us (although it is simpler to implement compared to pay.ubq.fi + github web flow auth).

To sum up

Overall it seems that we should refactor pay.ubq.fi to be a backend app and implement option 1. I will create the issue today.

P.S. Docs for future reference:

@0x4007
Copy link
Member

0x4007 commented Jan 22, 2024

@rndquu small update but work.ubq.fi has a serverless (Supabase) GitHub login already implemented. I use this to make authenticated requests to load all the issue details.

One part that isn't clear to me, but must the bot invalidate all of the posted permits across all of the GitHub issues first?

Ideally if the user can somehow invalidate them instead of Ubiquity, this would be cheaper for us from a gas perspective.

Once they are all invalidated, I assume only then it is safe to generate the new, rolled up permit?

@0x4007
Copy link
Member

0x4007 commented Jan 22, 2024

@whilefoo this seems not too different from the last project you implemented, the NFT claims. Perhaps you can finish this up quickly this week?

@rndquu
Copy link
Member Author

rndquu commented Jan 23, 2024

One part that isn't clear to me, but must the bot invalidate all of the posted permits across all of the GitHub issues first?

The latest bot's release supports rolled up permits stored in the DB, right?

@0x4007
Copy link
Member

0x4007 commented Jan 23, 2024

If a rolled up permit is technically the same as a normal permit (but in practice we modify the total compensation amount to be the sum of the amounts of the other invalidated permits) then yes the bot supports rolled up permits.

public.permits schema

    id integer,
    created timestamp with time zone not null default now(),
    updated timestamp with time zone null,
    amount text not null,
    nonce text not null,
    deadline text not null,
    signature character(132) not null,
    token_id integer null,
    partner_id integer null,
    beneficiary_id integer not null,
    transaction character(66) null,
    location_id integer null,

The issue is that the functional permits are encoded and embedded in the pay.ubq.fi comment link on GitHub, so even if we remove Supabase entirely from the equation, the contributor can still use the valid permit information from the GitHub comment.

@rndquu
Copy link
Member Author

rndquu commented Jan 25, 2024

Once they are all invalidated, I assume only then it is safe to generate the new, rolled up permit?

How rolled up permit generation works under the hood?

Suppose user solves 2 issues worth 10 USD and 20 USD. In the public.permits.amount field the 30 value is set (10 USD + 20 USD). When rolled-up permit is generated then:

  1. Rolled-up permit worth 30 USD is generated
  2. public.permits.amount is set to 0 so on next permit generation user won't be able to abuse the service in any way

Right?

@0x4007
Copy link
Member

0x4007 commented Jan 25, 2024

public.permits.amount is set to 0 so on next permit generation user won't be able to abuse the service in any way

  • We have two other tables of interest credits and debits aside from the permits table. The general idea is to keep a ledger for every contributor.
    • Credits: the original idea was that the credits table can also be for virtual "experience points" meaning no token reward, meaning no permit.
      • This is useful when partners want to manually modify the XP of a contributor.
      • I imagine that most credits will be associated with a permit.
    • Debits: this is useful for re-opened issues and applying penalties. Apparently can also be useful for permit invalidation/rollups.
    • Permits: this is strictly to store every permit generated by the system. This is essential for financial accounting.

Perhaps there is a simpler way to design this ledger (like a single table with positive and negative amounts representing debits and credits? It is not clear to me at the moment if this will be sufficient for all the envisioned scenarios. Also it will be far less efficient with many empty cells for those without permits.)

How rolled up permit generation works under the hood?

I'm assuming something like this, but I have mixed feelings on waiting for a transaction to complete before proceeding with the operation (I feel like there might be an entirely different and more streamlined approach that I can't think of right now) #139 (comment)

@whilefoo
Copy link
Contributor

@whilefoo this seems not too different from the last project you implemented, the NFT claims. Perhaps you can finish this up quickly this week?

When I implemented NFT claim I made support for multiple tokens that can be either ERC20 or ERC721


We could delay permit generation until the user goes to pay.ubq.fi logins with Github and requests payout, then we generate a permit based on credits and debits. Then we don't need to do invalidation and rolled-up permits, but that might make it less transparent where you got that amount from.

Also it will be a problem with multi token support because different tokens have different USD exchange rates so we would need to keep track of credit and debits for each user and for each token.
If bounty hunter's issue is reopened a penalty of 0.1 ETH (which is 219 USD) is issued but then he completes a bounty for 200 DAI, should we check the price of ETH and deduct 200$ worth of ETH from the penalty?

@0x4007
Copy link
Member

0x4007 commented Jan 25, 2024

We could delay permit generation until the user goes to pay.ubq.fi logins with Github and requests payout, then we generate a permit based on credits and debits. Then we don't need to do invalidation and rolled-up permits,

I think this is the best approach.

but that might make it less transparent where you got that amount from.

We can keep track in our database for auditing reasons.

Also it will be a problem with multi token support because different tokens have different USD exchange rates so we would need to keep track of credit and debits for each user and for each token.

If bounty hunter's issue is reopened a penalty of 0.1 ETH (which is 219 USD) is issued but then he completes a bounty for 200 DAI, should we check the price of ETH and deduct 200$ worth of ETH from the penalty?

I don't have a clear vision/solution for this but I see two paths:

  1. We whitelist specific tokens that can be used with the system (lame because new startup DAOs can't use their gov tokens out the gates) but this will allow us to do USD price conversions with fairly liquid tokens (we shouldn't see crazy volatility that breaks accounting)

  2. We allow any tokens for settlement (great!) but disable price conversions. We should keep accounting in the same tokens. So if they are owed 0.1 WETH but a task wants to pay 200 DAI, the permit would be generated for 0.1 WETH (questionable UX for partners)

Anyways this requires more thought. Maybe the simple answer is to only enable penalties on whitelisted tokens?

Excellent questions!

@rndquu
Copy link
Member Author

rndquu commented Jan 29, 2024

I don't think bounty hunters will be happy getting payout permits in such volatile tokens as ETH. The simplest/fastest approach seems to allow any tokens for settlement but disable USD conversion.

It also not clear where to fetch the PARTNER_TOKEN/USD quote since there may be cases when there is no one for a newly deployed token.

@0x4007
Copy link
Member

0x4007 commented Jan 29, 2024

We could delay permit generation until the user goes to pay.ubq.fi logins with Github and requests payout, then we generate a permit based on credits and debits. Then we don't need to do invalidation and rolled-up permits,

I think this is the best approach.

Counterpoint: I realized that this centralizes banking further and may appeal less to decentralization maxis. Posting payment permits is more tedious to deal with but more "decentralization friendly"

Copy link

ubiquibot bot commented Mar 5, 2024

! action has an uncaught error

@rndquu
Copy link
Member Author

rndquu commented Mar 14, 2024

Closing it until the new specs are ready ubiquity/ubiquibot#753 (comment)

@rndquu rndquu closed this as not planned Won't fix, can't repro, duplicate, stale Mar 14, 2024
Copy link

ubiquibot bot commented Mar 14, 2024

# Issue was not closed as completed. Skipping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants