Skip to content
This repository has been archived by the owner on Sep 19, 2024. It is now read-only.

Bounty rewards consolidation #714

Merged

Conversation

devpanther
Copy link
Contributor

@devpanther devpanther commented Sep 4, 2023

Resolves #704

There are 2 exceptions:

Issue solver shouldn't get issue commenter rewards - QA 1
Issue creator shouldn't get issue commenter rewards - QA 2

Assignee and Creator Reward Consolidated - QA 3

All QA (Including PR Review): one-two-test/one#12

Merging Reviewer and Commenter: one-two-test/one#12 (comment)

@netlify
Copy link

netlify bot commented Sep 4, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit e6741bd
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/6509e786de47a00008cd5691
😎 Deploy Preview https://deploy-preview-714--ubiquibot-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@devpanther
Copy link
Contributor Author

@rndquu

rndquu
rndquu previously approved these changes Sep 7, 2023
@rndquu rndquu requested review from web4er and whilefoo September 7, 2023 20:51
Copy link
Collaborator

@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.

Currently I feel like the code is messy and unreadable. Could you refactor it to make it easier to read? I feel like the names for some variables and functions are off.

Ideally I'd like to see a top level function incentivesCalculation that first checks if all conditions are met (not empty private key, issued closed as completed, is bounty, autopay is not off, and so on), then it should get all the issue comments (to avoid retrieving them multiple times).

After that it should call three function calculateIssueCreatorReward, calculateIssueConversationReward and calculateIssueAssigneeReward, all of them receive comments and return something like { user_id, username, reward }[], then the main function can aggregate all rewards together, apply multipliers, deduct penalties, retrieve user's wallet and generate permits.
I hope you understand what I mean. Let me know if you have questions

@devpanther
Copy link
Contributor Author

I hope you understand what I mean

I get what you mean, I thought about it and creating a function to aggregate all makes sense.

@devpanther
Copy link
Contributor Author

@whilefoo check it out now

Copy link
Collaborator

@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.

can you avoid using ! assertion?

src/handlers/comment/handlers/index.ts Outdated Show resolved Hide resolved
src/handlers/payout/action.ts Outdated Show resolved Hide resolved
@devpanther
Copy link
Contributor Author

can you avoid using ! assertion?

When they're null, the error is returned and it doesn't get to the functions anyway.

Only way to get rid of it is create an if else statement to check every value and it's going to be messy

@devpanther
Copy link
Contributor Author

Let me handle the merge #657 first.

I've merged #657 into this

@wannacfuture
Copy link
Contributor

Let me handle the merge #657 first.

I've merged #657 into this

will be great if you can consolidate rewards on pr reviewers on this pr. 👍

@devpanther
Copy link
Contributor Author

will be great if you can consolidate rewards on pr reviewers on this pr. 👍

Done already

@wannacfuture
Copy link
Contributor

will be great if you can consolidate rewards on pr reviewers on this pr. 👍

Done already

Cool, could you provide a QA for consolidation of all?

wannacfuture
wannacfuture previously approved these changes Sep 15, 2023
@devpanther
Copy link
Contributor Author

@wannacfuture I made some more changes to merge the reviewer and commenter, unlike the last permit.

Because it can only be one permit per user.

@devpanther
Copy link
Contributor Author

Let me handle the merge #657 first.

What's the update @0xcodercrane

src/handlers/payout/action.ts Outdated Show resolved Hide resolved
src/handlers/comment/handlers/index.ts Show resolved Hide resolved
@0xcodercrane
Copy link
Contributor

Let me handle the merge #657 first.

What's the update @0xcodercrane

It's been merged but waiting for other reviewer's approval.

@devpanther
Copy link
Contributor Author

It's been merged but waiting for other reviewer's approval.

Taking too long

0xcodercrane
0xcodercrane previously approved these changes Sep 19, 2023
@0xcodercrane
Copy link
Contributor

I still can see your requests have not been resolved yet @whilefoo.

@devpanther
Copy link
Contributor Author

I still can see your requests have not been resolved yet @whilefoo.

I've resolved his requests, except he has another.

@devpanther
Copy link
Contributor Author

@0xcodercrane ping

@0xcodercrane 0xcodercrane merged commit 3966177 into ubiquity:development Sep 21, 2023
7 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add bounty rewards consolidation
5 participants