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

September 2023 Update Adjustments #818

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

whilefoo
Copy link
Collaborator

Resolves #798

@netlify
Copy link

netlify bot commented Sep 23, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit 326b471
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/651c6bb6271e400008ea0ed6
😎 Deploy Preview https://deploy-preview-818--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.

0x4007
0x4007 previously approved these changes Sep 23, 2023
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

Looks solid is that the whole spec? Hard to tell from mobile

@whilefoo
Copy link
Collaborator Author

I made some additional adjustments

@0x4007
Copy link
Member

0x4007 commented Sep 24, 2023

My question still stands

@whilefoo
Copy link
Collaborator Author

It should be all now

src/handlers/comment/handlers/ask.ts Outdated Show resolved Hide resolved
src/handlers/comment/handlers/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

AmountInBigNumber instead I think is more expressive

const claimUrlRegex = new RegExp(`\\((${permitBaseUrl}\\?claim=\\S+)\\)`);
const permitCommentIdx = comments.findIndex((e) => e.user.type === UserType.Bot && e.body.match(claimUrlRegex));
const claimUrlRegex = new RegExp(`\\((${permitBaseUrl}\\S+)\\)`);
const permitCommentIdx = comments.findIndex((e) => e.user.type === UserType.Bot && e.body.match(claimUrlRegex) && e.body.includes("Task Assignee Reward"));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to call this Assignee Reward because task is implicit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We shouldn't change it because it will break backwards compatibility. We need to implement comment metadata first and then we can change format

@@ -148,8 +154,8 @@ export const calculateIssueCreatorReward = async (incentivesCalculation: Incenti
incentivesCalculation.paymentPermitMaxPrice
);

if (!result || !result.account || !result.amountInETH) {
throw new Error("Failed to generate permit for issue creator because of missing account or amountInETH");
if (!result || !result.account || !result.rewardInTokens) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's as expressive as inBigNumber etc it implies 1e18 format

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's not in 1e18 format, but with decimals like 5.235

src/handlers/push/index.ts Show resolved Hide resolved
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.

September 2023 Update Adjustments
2 participants