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

dApp staking v3 #1023

Merged
merged 77 commits into from
Jan 8, 2024
Merged

dApp staking v3 #1023

merged 77 commits into from
Jan 8, 2024

Conversation

bobo-k2
Copy link
Contributor

@bobo-k2 bobo-k2 commented Nov 15, 2023

Pull Request Summary

I know this is a huge PR, please take your time to review functionality and code. Let me know if there are unclear things or weird implementations.

User documentation is on it's way, but use of the feature shuold be strait forward.

Dapp staking v3 UI implementation

Check list

  • contains breaking changes
  • adds new feature
  • modifies existing feature (bug fix or improvements)
  • relies on other tasks
  • documentation changes
  • tested on mobile devices

Copy link

github-actions bot commented Nov 15, 2023

Visit the preview URL for this PR (updated for commit 2e6263e):

https://astar-apps--pr1023-feat-dapp-staking-v3-hb03lgyi.web.app

(expires Mon, 15 Jan 2024 10:55:47 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: dd76fe72958fe2910fef9d53f0b4539b82b849db

impelcrypto and others added 6 commits November 16, 2023 16:42
* Prevents UnavailableStakeFunds

* rephrase

* Prevent tooManyStakedContracts, unclaimedRewardsFromPastPeriods

* less logs

* unstake Prevents UnstakeAmountTooLarge ZeroAmount Disabled

* UnclaimedRewardsFromPastPeriods
* Voting page created

* Voting page - part 1

* Vote logic

---------

Co-authored-by: impelcrypto <impelcrypto@gmail.com>
* vote and dictionary changes

* fix value and more

* vote checks
src/staking-v3/components/DiscoverV3.vue Outdated Show resolved Hide resolved
src/staking-v3/components/DiscoverV3.vue Outdated Show resolved Hide resolved
src/staking-v3/components/Amount.vue Outdated Show resolved Hide resolved
src/staking-v3/components/DiscoverV3.vue Outdated Show resolved Hide resolved
<span> {{ $t('stakingV3.edit') }} </span>
</div>
<div class="box--links">
<!-- Todo: add links -->
Copy link
Member

Choose a reason for hiding this comment

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

Should we add the logic for owner page in this PR? If so, let's add the links

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave this for the future PR

src/staking-v3/components/FeatureDapp.vue Outdated Show resolved Hide resolved
src/staking-v3/components/FeatureDapp.vue Outdated Show resolved Hide resolved
</button>

<div class="column--action">
<!-- TODO: add logic -->
Copy link
Member

Choose a reason for hiding this comment

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

When and how do we implement the logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave this for the future PR

src/staking-v3/components/leaderboard/noEntry.vue Outdated Show resolved Hide resolved
src/staking-v3/components/leaderboard/noEntry.vue Outdated Show resolved Hide resolved
* @param senderAddress Address of the request sender.
* @param successMessage Message to be displayed on the call success.
*/
claimUnstakeAndUnlock(
Copy link
Member

Choose a reason for hiding this comment

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

I haven't noticed a function that only calls unstake - is this supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not supported, since we don't have that feature on the portal.

Comment on lines +101 to +102
* @param unstakeFromAddress Address of the contract to be unstaked from.
* @param unstakeAmount Amount of tokens to unstake.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a typo or the function also does unstake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is magic function which batches a lot of calls, depending what user selected on voting page:

  • claim staker and bonus rewards, if any
  • locks tokens,
  • unstakes tokens in case of nomination transfer
  • stakes tokens

I updated the method comment a bit

Comment on lines 175 to 180
if (ledger.staked.era <= era) {
stakedSum += ledger.staked.voting + ledger.staked.buildAndEarn;
}
if (ledger.stakedFuture && ledger.stakedFuture.era <= era) {
stakedSum += ledger.stakedFuture.voting + ledger.stakedFuture.buildAndEarn;
}
Copy link
Member

Choose a reason for hiding this comment

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

Should this be if & else if perhaps?

The way it is now, if I understand correctly, the staked_future might get included at the same time as the staked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Fixed.


// *** 3. Calculate rewards.
const multiplier = await this.getMultiplier();
for (let spanIndex = firstSpanIndex; spanIndex <= lastSpanIndex; spanIndex++) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using spanIndex+++, you should use this:

        /// Maximum length of a single era reward span length entry.
        #[pallet::constant]
        type EraRewardSpanLength: Get<u32>;

Unless I'm mistaken, the current approach will result in many empty reads?

for (let era = span.firstEra; era <= span.lastEra; era++) {
const staked = claimableEras.get(era);
if (staked) {
const spanIndex = era - span.firstEra;
Copy link
Member

Choose a reason for hiding this comment

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

Nothing logically wrong, but the name spanIndex confused me a bit since it's actually just the eraIndex in the span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

Comment on lines +377 to +384
// Unstake tokens
if (unstakeAmount > BigInt(0)) {
const unstakeCall = await this.dappStakingRepository.getUnstakeCall(
unstakeFromAddress,
Number(ethers.utils.formatEther(unstakeAmount.toString()))
);
calls.push(unstakeCall);
}
Copy link
Member

Choose a reason for hiding this comment

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

Why are tokens unstaked here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tokens are unstaked if user wants to move staked tokens to some other dApp (nomination transfer)

// and if stake amount refers to the past period.
if (
info.loyalStaker &&
protocolState &&
Copy link
Member

Choose a reason for hiding this comment

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

Just out of interest, when can this be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can happen in very rare case of communication error.


tierRewards.forEach((tierReward, index) => {
if (tierReward) {
const dApp = tierReward?.dapps.find((d) => d.dappId === dapp.id);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if there's a ready function out-of-the-box for JavaScript's Array, but dapps as sorted according to the id, so you could use binary_search to speed this action up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, no binary search in JS. I might implement it later. I am sure there are different implementations available on the internet.
find method used here has O(n) complexity.

Comment on lines 680 to 683
private async getMultiplier(): Promise<bigint> {
const metadata = await this.metadataRepository.getChainMetadata();
return BigInt(10 ** metadata.decimals);
}
Copy link
Member

Choose a reason for hiding this comment

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

Dumb question - I noticed this being used in the code but don't understand why it's needed. Could you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because I didn't find a better way to divide two BigInt numbers. Lets say you have two BigInt numbers a and b and a<b, than a / b = 0.
I modified formula a bit (a * multiplier / b) / multiplier

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see!

So for example this:

 (((info.staked.voting * multiplier) / periodEndInfo.totalVpStake) *
              periodEndInfo.bonusRewardPool) /
            multiplier;

is essentially: info.staked.voting * multiplier / periodEndInfo.totalVpStake * periodEndInifo.bonusRewardPool * multiplier

To make it more in line with how backend works, I'd suggest:
info.staked.voting * periodEndInfo.bonusRewardPool / periodEndInfo.totalVpStake

This isn't 100% accurate as runtime does it, but it's very close.
And this way you don't need the multiplier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thank you

Copy link
Contributor

@Kahonnohak Kahonnohak left a comment

Choose a reason for hiding this comment

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

Lets go!

bobo-k2 and others added 5 commits January 5, 2024 09:15
* add project icon background

* add project icon background to the owner page

* update all vote/stake button background

* fix app height

* move the register button to the assets page

* fix assets page container

* remove the license section from the project page

* made the header scroll

* add small dapp header

* fix v2 style

* remove the register banner from the assets page for now

* fix vote page layout

* fix vote period background

* adjust small header style and heart icon button

* update maintenance mode style

* update
* add migrate support style

* fix lang file
* feat: added useAprV3

* wip: staker rewards

* wip: staker rewards (2)

* wip: staker rewards (3)

* wip: staker rewards (4)

* wip: staker rewards (5)

* wip: staker rewards (6)

* wip: staker rewards (7)

* wip: staker rewards (8)

* feat: added basic rewards in the voting section

* fix: clean up

* fix: refactor

* feat: added bonus APR

* fix: clean up

* fix: set the periodsPerCycle from network name temporarily

* fix: set the periodsPerCycle from network name temporarily (2)
@bobo-k2 bobo-k2 merged commit ae4e280 into main Jan 8, 2024
7 checks passed
@bobo-k2 bobo-k2 deleted the feat/dapp-staking-v3 branch January 8, 2024 13:34
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.

None yet

6 participants