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

Html comments #545

Merged
merged 20 commits into from
Aug 22, 2023
Merged

Html comments #545

merged 20 commits into from
Aug 22, 2023

Conversation

whilefoo
Copy link
Collaborator

@whilefoo whilefoo commented Jul 21, 2023

Resolves #496

@netlify
Copy link

netlify bot commented Jul 21, 2023

Deploy Preview for ubiquibot-staging ready!

Name Link
🔨 Latest commit ee91997
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-staging/deploys/64e48b71e0d0e900084e165b
😎 Deploy Preview https://deploy-preview-545--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.

@whilefoo
Copy link
Collaborator Author

@pavlovcik I'm not quite sure if I understand how the reward should be calculated.

If my understanding is correct: if the config has li: 1, then every li found in the comment is worth 1. Then the text inside the li should be counted by words and if the config has word: 0.1 every word in the li is worth 0.1.

If config has h1: 0 should it still count the words inside?

@whilefoo
Copy link
Collaborator Author

@pavlovcik still waiting

@0x4007
Copy link
Member

0x4007 commented Jul 27, 2023

Yes double count please thank you for your follow up.

@whilefoo whilefoo marked this pull request as ready for review July 29, 2023 20:53
@whilefoo
Copy link
Collaborator Author

Proposed config format change:

incentives:
  comment:
    elements:
      code: 5
      img: 5
      h1: 1
      li: 0.5
      a: 0.5
      blockquote: 0
    totals:
      word: 0.1

This way is easier for the type system.

@0x4007
Copy link
Member

0x4007 commented Jul 30, 2023

That seems fine to me.

src/bindings/config.ts Outdated Show resolved Hide resolved
src/configs/price.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.

Code looks pretty good.

src/helpers/issue.ts Outdated Show resolved Hide resolved
@0x4007 0x4007 requested review from rndquu and 0xcodercrane July 30, 2023 14:13
@rndquu
Copy link
Member

rndquu commented Jul 30, 2023

@whilefoo Could you show a QA issue of how it works in real life?

Copy link
Contributor

@0xcodercrane 0xcodercrane left a comment

Choose a reason for hiding this comment

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

@whilefoo please fix ci/cd issue first.

@whilefoo
Copy link
Collaborator Author

whilefoo commented Aug 1, 2023

QA issue: whilefoo#10

Because we are calculating with float we get 22.400000000000002 WXDAI. One solution is to round up to 2 decimals to fix this.

@0xcodercrane
Copy link
Contributor

@Draeieg would you verify the QA link?
@whilefoo would you resolve the conflicts?

@0x4007
Copy link
Member

0x4007 commented Aug 2, 2023

One solution is to round up to 2 decimals to fix this.

I'm surprised there isn't a more proper way to instruct JavaScript to calculate this accurately. It seems like a workaround to round up to two decimals for this but if this is the only solution proposal let's do it.

@whilefoo
Copy link
Collaborator Author

whilefoo commented Aug 2, 2023

Another solution would be using a library like decimal.js

@whilefoo
Copy link
Collaborator Author

QA: ubiquity-whilefoo#4

@0x4007
Copy link
Member

0x4007 commented Aug 17, 2023

For this PR I will just match the text to skip already generated permits and we can do metadata in another bounty.

Agreed I'm also ready to merge this in!

wannacfuture
wannacfuture previously approved these changes Aug 17, 2023
@0x4007 0x4007 mentioned this pull request Aug 17, 2023
3 tasks
src/handlers/payout/post.ts Show resolved Hide resolved
src/handlers/payout/post.ts Show resolved Hide resolved
@web4er
Copy link
Contributor

web4er commented Aug 17, 2023

Github single comment limit is 65536 characters. Looked into the possibility of an exploit before a user completes a task and makes a big comment. It went well. The reward is a small amount with configs being used in this PR.

Tested max words, generated 1.5045 WXDAI incentive. web4erOrg#54
Tested high-worth element, img, 610 imgs 62220 Characters, generated 5.0325 WXDAI incentive. web4erOrg#55

@0x4007
Copy link
Member

0x4007 commented Aug 17, 2023

Github single comment limit is 65536 characters. Looked into the possibility of an exploit before a user completes a task and makes a big comment. It went well. The reward is a small amount with configs being used in this PR.

Tested max words, generated 1.5045 WXDAI incentive. web4erOrg#54 Tested high-worth element, img, 610 imgs 62220 Characters, generated 5.0325 WXDAI incentive. web4erOrg#55

Thanks for testing for exploits. As a heads up, once things are ironed out (we test comment incentives in production for awhile) I plan to crank up those numbers quite a bit.

@0x4007 0x4007 requested a review from web4er August 20, 2023 09:54
@0x4007 0x4007 linked an issue Aug 20, 2023 that may be closed by this pull request
wannacfuture
wannacfuture previously approved these changes Aug 21, 2023
@wannacfuture
Copy link
Contributor

I think we need to get this PR merged ASAP for comment incentives.
We can work on hotfixes if there is something wrong. @pavlovcik

@0x4007
Copy link
Member

0x4007 commented Aug 21, 2023

I think we need to get this PR merged ASAP for comment incentives.

We can work on hotfixes if there is something wrong. @pavlovcik

I agree just as long as this is mostly functional and passes some QA (looks like it did)

I think it just depends on @web4er to approve so we can merge!

Copy link
Contributor

@web4er web4er left a comment

Choose a reason for hiding this comment

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

I think it just depends on @web4er to approve so we can merge!

Looks good.
QA: normal comment incentive working ok: web4erOrg#60
QA: generating permits when PR is merged web4erOrg#64
QA: stopping permits when PR is merged but amount is above payment-permit-max-price web4erOrg#62
QA: payment-permit-max-price is being respected now. But it is still leaving and empty comment incentive reward comment. web4erOrg#61 I will approve it as team is planning merge soon and apply hotfix.

@0x4007 0x4007 added this pull request to the merge queue Aug 22, 2023
Merged via the queue into ubiquity:development with commit 12777f0 Aug 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Explicitly state what the permits are for Refactor Config: mdast element keys as HTML entities / ref
7 participants