ubiquity-os-marketplace / text-conversation-rewards

1 stars 27 forks source link

Add a new column to the table that displays the fee per line item transparently #161

Open gentlementlegen opened 1 month ago

gentlementlegen commented 1 month ago

New QA: https://github.com/Meniole/conversation-rewards/issues/23#issuecomment-2413032702

The failing tests are the ones involving the RPCs.

Thanks for the QA

As part of a separate task, I think we should consider adding a new column to the table that displays the fee per line item transparently

Originally posted by @0x4007 in https://github.com/ubiquity-os-marketplace/text-conversation-rewards/issues/159#issuecomment-2415724852

jaykayudo commented 1 month ago

/start

ubiquity-os-beta[bot] commented 1 month ago
Deadline Thu, Oct 17, 3:04 AM UTC
Beneficiary 0x830E2a4D7714989361BA76d3eA9C49e093f0A04C

[!TIP]

  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.
jaykayudo commented 1 month ago

@0x4007 @gentlementlegen according to this code section, the reward is the amount gotten by multiplying the feeRate and the original. In that sense, if the feeRate is 20% (0.2) and the original reward is 100, then rewardAfterFee is 20. wouldn't that make the fee 80? is this how it is supposed to be?

gentlementlegen commented 1 month ago

It should be that a 20% fee rate is applied to the total, so the final result should be total: 80 and feeRate: 20%. Did you see something different?

jaykayudo commented 1 month ago

Yes. From the algorithm, if i am looking at it correctly, feeRate: 20% and total: 20. when you multiplied the feeRate to the total, you assigned the value directly to the total instead of substracting that value from the original total.

jaykayudo commented 1 month ago

Yes. From the algorithm, if i am looking at it correctly, feeRate: 20% and total: 20. when you multiplied the feeRate to the total, you assigned the value directly to the total instead of substracting that value from the original total.

I think i got where my confusion is coming from. i think the value is wrong for the feeRate. I think the value should be

result[key].feeRate = new Decimal(1).minus(feeRateDecimal).toNumber();

or

result[key].feeRate =new Decimal(env.PERMIT_FEE_RATE).div(100).toNumber()

What do you think?

gentlementlegen commented 1 month ago

I think the correct calculation is

result[key].feeRate = new Decimal(env.PERMIT_FEE_RATE).toNumber()

since the PERMIT_FEE_RATE directly represents what percentage was applied. But that's only affecting the percentage displayed, the result for the rewards is correct.

jaykayudo commented 1 month ago

I think the correct calculation is

result[key].feeRate = new Decimal(env.PERMIT_FEE_RATE).toNumber()

since the PERMIT_FEE_RATE directly represents what percentage was applied. But that's only affecting the percentage displayed, the result for the rewards is correct.

Yes.. it only affects the displayed fees. Should i make the change within this issue PR since it depends on the correct feeRate or you’ve got it ?

gentlementlegen commented 1 month ago

You can do it. Also we should wrap the <a/> with spaces because currently it doesn't have spacing around.

image
jaykayudo commented 1 month ago

Okay

jaykayudo commented 1 month ago
new Decimal(env.PERMIT_FEE_RATE).toNumber()

I just realized something. with this implementation, the feeRate will be 20 instead of 0.2. Is this how it should work from now on?

gentlementlegen commented 1 month ago

It's a percentage so 20% makes sense to me.

jaykayudo commented 1 month ago

It's a percentage so 20% makes sense to me.

That also means that this line of code will change as well. Yeah?

jaykayudo commented 1 month ago

@gentlementlegen I have updated the PR. The feeRate is now in percentage.

ubiquity-os-beta[bot] commented 3 weeks ago

Passed the deadline and no activity is detected, removing assignees: @jaykayudo.

0x4007 commented 3 weeks ago

I think we need to set our settings for disqualifier to be slower like 7 days for follow up.

This is because the new algorithm will divide by priority level.

7 days / 2 = 3 days 12 hours 
7 days / 3 = 2 days 8 hours 
7 days / 4 = 1 day 18 hours 
7 days / 5 = 1 day 9 hours 36 minutes 
gentlementlegen commented 3 weeks ago

And also now we instantly remove if there is no PR opened which is why more people get kicked out.