Skip to content

Conversation

@meiersi-da
Copy link
Contributor

Pull Request Checklist

Cluster Testing

  • If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
  • If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

  • Include any change that might be observable by our partners or affect their deployment in the release notes.
  • Specify fixed issues with Fixes #n, and mention issues worked on using #n
  • Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

  • Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

Copy link
Contributor

@moritzkiefer-da moritzkiefer-da left a comment

Choose a reason for hiding this comment

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

Thanks or creating this! I made a pass and it looks plausible but I think I need some more clarification on how this affects the overall minting flows to get confidence that this works. I tried to spell out parts of that in a comment

--
-- Determined using SV voting.
mintingAllowance : Decimal -- ^ Unused minting allowance for SV rewards.
unconstrainedStake : Decimal -- ^ Amount of stake that is not currently locked or constrained.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I understand what this is. You say it's the amount not locked up. But then you add the relock amount to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry... its staked amount that is neither being "onboarded" nor "offboarded". It's definitely locked. Will fix the comment.

Copy link

Choose a reason for hiding this comment

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

Isn't this always the quantity in an associated LockedAmulet - the amounts in the stakes? If so, is it a good idea to keep this amount in the StakerState?

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 was meant as a technical optimization to avoid having to track many unlocked stakes, which haven't been reclaimed. It's though something to reconsider when doing the proper implementation that deals with the in- and out-flow of (Locked)Amulet.

-- Copyright (c) 2024 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved.
-- SPDX-License-Identifier: Apache-2.0

-- | Staking for the SV rewards tranche Splice Amulet.
Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be a lot of implicit assumptions in this code that are not spelled out which make it quite hard for me to follow how the high-level minting and governance flow change. I think I got it now but let's spell it out more explicitly. Here is what I understood:

  1. All SV weights are now tracked through governance including beneficiary weights. So no more "GSF has a very large weight and how they split it up is technically up to them and only defined through local configuration".
  2. SV Reward Minting (and third party staking minting) does not work through creating coupons and minting them once per round. Instead you track the minting allowance and people can withdraw from that at their discretion.
  3. For SVs we do want them to do something per round to get rewards so the current mechanism through activity records is replaced by skippedSvs which is filled in through some undetermined mechanism.
  4. Once per round the SVs call updateMintingAllowances through a confirmation based process (likely with some SVs only committing to a hash and sharding it across SVs or something if the number of stakers grows large).
  5. Not fully clear to me how the ghost SVs translate. I guess you have some dummy party that accrues a balance that the SVs can mint from instead of minting from unclaimed rewards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the quick turnaround @moritzkiefer-da ; and the summary below. I've answered inline; and will ping @bame-da for a review before adjusting the code to inline your commentary in an appropriate form.

There seem to be a lot of implicit assumptions in this code that are not spelled out which make it quite hard for me to follow how the high-level minting and governance flow change. I think I got it now but let's spell it out more explicitly. Here is what I understood:

  1. All SV weights are now tracked through governance including beneficiary weights. So no more "GSF has a very large weight and how they split it up is technically up to them and only defined through local configuration".

Yes. I overlooked the requirement to now track all of them on-ledger; and thus have not spelled it out. I also realize that this will very likely require tracking the state of escrowed rewards from milestone-based SV rewards.

I'd expect that these are tracked in a separate escrowedMintingAllowance. What's unclear is whether they immediately count as lifetimeRewards or only once they get released form escrow. I suspect the latter.

@bame-da: what's your view?

  1. SV Reward Minting (and third party staking minting) does not work through creating coupons and minting them once per round. Instead you track the minting allowance and people can withdraw from that at their discretion.

Yes.

  1. For SVs we do want them to do something per round to get rewards so the current mechanism through activity records is replaced by skippedSvs which is filled in through some undetermined mechanism.

Yes, the strawman I have in mind is a comparison between the last status update and the round closing time to show that they have been active after roundClosesAt - 2 * tickDuration.

  1. Once per round the SVs call updateMintingAllowances through a confirmation based process (likely with some SVs only committing to a hash and sharding it across SVs or something if the number of stakers grows large).

Yes. Also the computation needs to be refactored a bit to take the form Summarize --> offLedgerSummary --> parallel updateStakerSate.

  1. Not fully clear to me how the ghost SVs translate. I guess you have some dummy party that accrues a balance that the SVs can mint from instead of minting from unclaimed rewards?

See above. I'd make the escrowedMintingAllowance explicit.

defaultStakingConfig = StakingConfig with
stakingDelay = days 30
unlockDelay = days 365
minStakeAmount = 100_000.0
Copy link

Choose a reason for hiding this comment

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

Why is this needed? For a fresh SV, the first round they mint, 80% may be less than 100k. Later on, nobody is going so stake anywhere between 0 and 20% of their historic total.

lifetimeRewards : Decimal -- ^ Total lifetime rewards earned by the staker.
exemptedLifetimeRewards : Decimal
-- ^ Portion of lifetime rewards that are exempted from staking requirements.
-- MAY be negative to reflect an exemption for future rewards.
Copy link

Choose a reason for hiding this comment

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

I don't understand this. Is this an absolute amount? Why would this be negative? Wouldn't it be > lifetimeRewards if it takes into account future rewards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, apologies. The difference liftetimeRewards - exemptedLifetimeRewards may be negative. Will fix.

-- MAY be negative to reflect an exemption for future rewards.
--
-- Determined using SV voting.
mintingAllowance : Decimal -- ^ Unused minting allowance for SV rewards.
Copy link

Choose a reason for hiding this comment

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

Isn't this just 1.0 - relockRatio? Under what circumstances would it be different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code uses this field to track both unminted rewards and unlocked stakes.

The reason for tracking a minting allowance is to avoid having the SVs mint for a coin owner. I realize though that the totals of unlocked stake and unminted coin should be tracked separately.

Even then the total unminted coin is different from 1.0 - relockRatio.

Side note: the state shown here is state that's tracked with signatory dso only. There's then a second process that mirrors that dso state to the actual parties. The split for that is due to the fact that observers can block a transaction if they haven't vetted the required packages. We do not want the DSO to get blocked, which is why we opt to do this kind of two-step issuance.

The alternative would be to make certain packages required to connect to a synchronzier.

-- Determined using SV voting.
mintingAllowance : Decimal -- ^ Unused minting allowance for SV rewards.
unconstrainedStake : Decimal -- ^ Amount of stake that is not currently locked or constrained.
stakes : [Stake] -- ^ Stakes with constraints.
Copy link

Choose a reason for hiding this comment

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

For consistency, name constrainedStakes and ConstrainedStake or some such?
It would make clearer that total stake = constrained + unconstrained. Right now, one needs to infer that as the naming could suggest that stakes is inclusive of unconstrainedStake.

Copy link

Choose a reason for hiding this comment

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

Actually, I'm not sure whether it is or isn't inclusive... The data type of Stake doesn't make sense to me.

deriving (Eq, Show)

-- | An individual stake with its associated metadata.
data Stake = Stake with
Copy link

Choose a reason for hiding this comment

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

This data type doesn't make sense to me.
Suppose I stake 100.0 at T1, then accumulate a bunch of stake through relock, then stake another 100.0 at T2, then accumulate a bunch more through relock to end up at a total of 250.0. I now request to unlock 51.0 at T3...

What will the stakes field contain? I'd imagine I have two elements with amount 100.0 and stakedAt T1 and T2. But I'd also expect an element with amount 51.0 and requestedUnlockAt T3. But what's the stakedAt of that last element?

Or if the idea is that at times T1 + stakingDelay and T2 + stakingDelay the elements disappear from stakes, and I can only unlock from unconstrainedStake, then I will never have both stakedAt and requestedUnlockAt set at the same time so maybe it should be a sum type like

data StakingConstraint
  = SCStakedAt Time
  | SCRequestedUnlockAt Time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or if the idea is that at times T1 + stakingDelay and T2 + stakingDelay the elements disappear from stakes, and I can only unlock from unconstrainedStake.

That was the core idea. The reason for having both fields is that a staker might still request to unlock before the stakedAt constraint runs out. I'd have imagined that the unlocking works by ordering the stakes for which no unlocked has yet been requested in reverse order of stakedAt, and adding the requestedUnlockAt constraint.

The last stake encountered in this process might get split into two stakes, as would be required in your example.

lifetimeRewards : Decimal -- ^ Total lifetime rewards earned by the staker.
exemptedLifetimeRewards : Decimal
-- ^ Portion of lifetime rewards that are exempted from staking requirements.
-- MAY be negative to reflect an exemption for future rewards.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, apologies. The difference liftetimeRewards - exemptedLifetimeRewards may be negative. Will fix.

--
-- Determined using SV voting.
mintingAllowance : Decimal -- ^ Unused minting allowance for SV rewards.
unconstrainedStake : Decimal -- ^ Amount of stake that is not currently locked or constrained.
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 was meant as a technical optimization to avoid having to track many unlocked stakes, which haven't been reclaimed. It's though something to reconsider when doing the proper implementation that deals with the in- and out-flow of (Locked)Amulet.

-- MAY be negative to reflect an exemption for future rewards.
--
-- Determined using SV voting.
mintingAllowance : Decimal -- ^ Unused minting allowance for SV rewards.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code uses this field to track both unminted rewards and unlocked stakes.

The reason for tracking a minting allowance is to avoid having the SVs mint for a coin owner. I realize though that the totals of unlocked stake and unminted coin should be tracked separately.

Even then the total unminted coin is different from 1.0 - relockRatio.

Side note: the state shown here is state that's tracked with signatory dso only. There's then a second process that mirrors that dso state to the actual parties. The split for that is due to the fact that observers can block a transaction if they haven't vetted the required packages. We do not want the DSO to get blocked, which is why we opt to do this kind of two-step issuance.

The alternative would be to make certain packages required to connect to a synchronzier.

deriving (Eq, Show)

-- | An individual stake with its associated metadata.
data Stake = Stake with
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or if the idea is that at times T1 + stakingDelay and T2 + stakingDelay the elements disappear from stakes, and I can only unlock from unconstrainedStake.

That was the core idea. The reason for having both fields is that a staker might still request to unlock before the stakedAt constraint runs out. I'd have imagined that the unlocking works by ordering the stakes for which no unlocked has yet been requested in reverse order of stakedAt, and adding the requestedUnlockAt constraint.

The last stake encountered in this process might get split into two stakes, as would be required in your example.

-- Third-party staker: issue from staking pool for their part of the total pool stake
-- Note that an SV staker that is offboarded from the SVs will become a third-party staker.
let stakedAmount = getStakedAmount stakerState
totalAllowance = stakingPoolMintingAllowance * (stakedAmount / totalThirdPartyStake)
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 likely not how it should work. The third-party stake should be weighted as well.

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.

4 participants