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

Liquidity mining #1851

Merged
merged 279 commits into from
Aug 27, 2021
Merged

Liquidity mining #1851

merged 279 commits into from
Aug 27, 2021

Conversation

kadenzipfel
Copy link
Contributor

@kadenzipfel kadenzipfel commented Apr 21, 2021

Closes: #1822
Closes: #1824
Closes: #1832
Closes: #1833
Closes: #1834

Testing (rinkeby):

Here are the markets that can be used for testing:

New markets for testing:

Test cases:

  • Successful market creation (Share tx so I can verify that staking contract was properly deployed)
    • Categorical
    • Scalar
  • Categorical market (see markets above)
    • Buy
    • Sell
    • Deposit
    • Claim rewards
    • Withdraw
    • Bond
    • Redeem
  • Scalar market (see markets above)
    • Buy
    • Sell
    • Deposit
    • Claim rewards
    • Withdraw
    • Bond
    • Redeem

@kadenzipfel kadenzipfel marked this pull request as draft April 21, 2021 22:24
@kadenzipfel kadenzipfel changed the title Add liquidity mining data to market details view Add liquidity mining data to market details view and list item view Apr 21, 2021
@kadenzipfel kadenzipfel changed the base branch from feature/1834 to master April 23, 2021 16:08
@kadenzipfel kadenzipfel changed the title Add liquidity mining data to market details view and list item view Add liquidity mining data to market details view, list item view and market pool view Apr 23, 2021
@pimato pimato self-requested a review April 24, 2021 09:04
Copy link
Contributor

@pimato pimato left a comment

Choose a reason for hiding this comment

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

I couldn`t wait to check this important PR out :) . It looks like you are using an svg prepared for the market details view which does not work well with the markets overview listItem rewards icon (borders are too thin):
Bildschirmfoto 2021-04-24 um 11 07 52

I´ve created a specific rewards icon with 16x16px so the borders are look like they supposed to:
rewards.svg.zip

@kadenzipfel kadenzipfel requested a review from pimato April 26, 2021 23:56
@pimato
Copy link
Contributor

pimato commented Apr 27, 2021

Hey Kaden, just checked and it seems that your test market has no active rewards program:
Bildschirmfoto 2021-04-27 um 14 31 37

@kadenzipfel
Copy link
Contributor Author

@kadenzipfel tried out the new markets and was getting one of those 'transaction will revert' cpk errors when trying to claim - To reproduce you can deposit some amount and then click and cancel 'Claim Rewards' until you get an error.

I think this might be similar to the sell issues we had where our off-chain calcs were a little bit high. In withdrawRewards if I add:

const totalRewardsAmount = claimableRewards[i].add(unclaimedRewards).mul(99999999).div(100000000)

It seems like the error goes away. What do you think?

I haven't been able to reproduce, but there shouldn't be room for a calculation issue since we're fetching data directly from the chain. To clarify, you reproduce by clicking claim then rejecting the tx and repeating until you get the error? Does it work if you wait a few seconds and try again?

@hexyls
Copy link
Contributor

hexyls commented Aug 24, 2021

mmmmm true, all data directly fetched from the chain 🤔 gotta see if I can reproduce again today but yeah what you wrote is how I was getting the error yesterday

edit: I can't figure out why this is happening 😩 more details: I'm using this market and I have 29.70 DAI out of the total 30.69 DAI liquidity. When I click claim rewards I get a cpk error... if I add await new Promise(r => setTimeout(r, 2000)) inside withdrawRewards the error goes away so must be some sort of timing thing...

@Mi-Lan
Copy link
Contributor

Mi-Lan commented Aug 25, 2021

Was just testing it! Claiming and depositing and withdrawing works good fore me. I found one issue when I claim rewards immediately after Im able to claim it again. Is this some issue with dust value or?
https://user-images.githubusercontent.com/33226956/130783970-4ae1e4e3-3a76-401a-9741-e03013087bad.mov

@pimato
Copy link
Contributor

pimato commented Aug 25, 2021

looks like there is an issue with showing the current APY with this market:
http://localhost:3000/#/0xe253df8dfc512b938d09ede69406f68185e13875

Bildschirmfoto 2021-08-25 um 14 10 11

@pimato
Copy link
Contributor

pimato commented Aug 25, 2021

can we reduce the decimals here to just two:
Bildschirmfoto 2021-08-25 um 14 54 22

@pimato
Copy link
Contributor

pimato commented Aug 25, 2021

I don`t think it is an issue with the 15 seconds block time as @kadenzipfel is expecting. I tried to claim rewards 4 minutes after my deposit:

nue-2.mov

@Mi-Lan
Copy link
Contributor

Mi-Lan commented Aug 25, 2021

I don`t think it is an issue with the 15 seconds block time as @kadenzipfel is expecting. I tried to claim rewards 4 minutes after my deposit:

nue-2.mov

Im able to reproduce it but only on categorical http://localhost:3001/#/0xe253df8dfc512b938d09ede69406f68185e13875/pool

@kadenzipfel
Copy link
Contributor Author

@hexyls @Mi-Lan @pimato I still have not been able to reproduce the bug but I have implemented hexyls' suggestion to reduce the total claim amount slightly. Hopefully this should generally prevent issues, but I think using the cpk for this kind of action will never be quite perfect, yet as long as the user can still claim after trying again I think it's ok to ignore

@hexyls
Copy link
Contributor

hexyls commented Aug 26, 2021

@kadenzipfel yeah no idea why but that does appear to resolve the issue 👍 curious if the problem goes away for anyone else

@pimato
Copy link
Contributor

pimato commented Aug 26, 2021

Test cases:

@pimato pimato requested review from Mi-Lan, Frankaus and pimato August 26, 2021 09:37
@Mi-Lan
Copy link
Contributor

Mi-Lan commented Aug 26, 2021

Test cases:

@kadenzipfel kadenzipfel marked this pull request as ready for review August 26, 2021 18:14
@kadenzipfel
Copy link
Contributor Author

New, short term markets for testing:

Resolution date is Sunday at 12:00am UTC, so staking rewards end Saturday at 12:00am UTC so make sure to test before then

@hexyls
Copy link
Contributor

hexyls commented Aug 27, 2021

Copy link
Contributor

@pimato pimato left a comment

Choose a reason for hiding this comment

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

Works!

@pimato pimato merged commit 2cfdf6b into master Aug 27, 2021
@pimato pimato deleted the feature/1833 branch August 27, 2021 13:20
@pimato pimato mentioned this pull request Aug 27, 2021
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment