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

Feat/permit fees #43

Merged
merged 13 commits into from
Jul 15, 2024
Merged

Conversation

rndquu
Copy link
Member

@rndquu rndquu commented Jun 20, 2024

Depends on #40

Resolves ubiquity-os/permit-generation#16

This PR introduces permit fees. When issue is closed as completed then additional permit will be generated for the treasury. Treasury must be a separate github account (i.e. registered with /wallet).

QA (no fees):
rndquu-org/test-repo#62 (comment)

QA (10% fee, i.e. PERMIT_FEE_RATE=10, PERMIT_TREASURY_GITHUB_USERNAME=rndquu3):
rndquu-org/test-repo#62 (comment)

2 permits (assignee reward + treasury fee) are generated in a DB:
Screenshot 2024-07-11 at 10 17 58

@rndquu
Copy link
Member Author

rndquu commented Jul 5, 2024

@0x4007 This PR generates a separate fee permit for the treasury address (along with our traditional permits for solving an issue and comments).

Example with 10% fee:

  1. User1 gets 90 WXDAI for solving an issue
  2. User2 gets 9 WXDAI for comments
  3. Treasury gets 11 WXDAI fee

The thing with this approach is that treasury address must have a github account (i.e. registered with the /wallet command). This simplifies code greatly. Is this approach fine?

@0x4007
Copy link
Member

0x4007 commented Jul 5, 2024

The thing with this approach is that treasury address must have a github account (i.e. registered with the /wallet command). This simplifies code greatly. Is this approach fine?

Oh I see. If it simplifies the code then yeah it probably is the best approach.

@rndquu
Copy link
Member Author

rndquu commented Jul 10, 2024

@gentlementlegen Help

There are 5 failing tests in this CI run which are failing with the supabaseUrl is required error.

More logs:

$ jest --setupFiles dotenv/config --coverage
FAIL tests/parser/permit-generation-module.test.ts
  ● permit-generation-module.ts › applyFees() › Should not apply fees if PERMIT_FEE_RATE is empty

    supabaseUrl is required.

      [35](https://github.com/ubiquibot/conversation-rewards/actions/runs/9872254093/job/27262322258#step:4:36) | export class PermitGenerationModule implements Module {
      [36](https://github.com/ubiquibot/conversation-rewards/actions/runs/9872254093/job/27262322258#step:4:37) |   readonly _configuration: PermitGenerationConfiguration = configuration.incentives.permitGeneration;
    > [37](https://github.com/ubiquibot/conversation-rewards/actions/runs/9872254093/job/27262322258#step:4:38) |   readonly _supabase = createClient<Database>(process.env.SUPABASE_URL, process.env.SUPABASE_KEY);
         |                                    ^
      [38](https://github.com/ubiquibot/conversation-rewards/actions/runs/9872254093/job/27262322258#step:4:39) |
      39 |   async transform(data: Readonly<IssueActivity>, result: Result): Promise<Result> {
      [40](https://github.com/ubiquibot/conversation-rewards/actions/runs/9872254093/job/27262322258#step:4:41) |     const payload: Context["payload"] & Payload = {

Somehow process.env.SUPABASE_URL and process.env.SUPABASE_KEY are not set although they are passed in the jest workflow and also set in my fork:
Screenshot 2024-07-10 at 13 25 34

Perhaps you see what's wrong?

@gentlementlegen
Copy link
Member

@rndquu When the Jest runs, since it's a fork, it doesn't have access to secrets: https://github.com/ubiquibot/conversation-rewards/actions/runs/9872254093/job/27262322258?pr=43#step:1:32
To fix this, two solutions:

  • mock supabase module entirely
  • give dummy values and intercept the requests with msw

@rndquu
Copy link
Member Author

rndquu commented Jul 11, 2024

@rndquu When the Jest runs, since it's a fork, it doesn't have access to secrets: https://github.com/ubiquibot/conversation-rewards/actions/runs/9872254093/job/27262322258?pr=43#step:1:32 To fix this, two solutions:

  • mock supabase module entirely
  • give dummy values and intercept the requests with msw

As far as I understand it does have access to secrets but only to those set in the forked instance.

Ok, thank you, I'll try one of the proposed solutions.

@rndquu rndquu marked this pull request as ready for review July 11, 2024 07:21
@rndquu rndquu marked this pull request as ready for review July 12, 2024 08:41
Copy link
Contributor

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good but I have another idea:

how about we add another variable fee (percentage) and totalAfterFee/totalBeforeFee and we leave other values unmodified? this way we can display a comment which includes information about the total reward, how much was the fee and total reward after the fee.

This would increase transparency so the user can clearly see how much the comments/task was worth and sees how much was the fee

@rndquu
Copy link
Member Author

rndquu commented Jul 13, 2024

this looks good but I have another idea:

how about we add another variable fee (percentage) and totalAfterFee/totalBeforeFee and we leave other values unmodified? this way we can display a comment which includes information about the total reward, how much was the fee and total reward after the fee.

This would increase transparency so the user can clearly see how much the comments/task was worth and sees how much was the fee

You mean adding those properties to the result data structure, right? I'll add it in a separate PR.

@rndquu rndquu merged commit 97a1a38 into ubiquity-os-marketplace:development Jul 15, 2024
3 checks passed
@rndquu rndquu deleted the feat/permit-fees branch July 15, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dollar permits with fees
4 participants