Joystream / orion

Atlas backend
GNU General Public License v3.0
7 stars 15 forks source link

Fix: `ProjectToken.RevenueSplitLeft` mapping #324

Closed zeeshanakram3 closed 7 months ago

zeeshanakram3 commented 7 months ago

This fix makes it possible to unstake participant tokens after the current revenue share is finalized and the new revenue share is started

WRadoslaw commented 7 months ago

Here is a whole function for a context:

export async function processRevenueSplitLeftEvent({
  overlay,
  event: {
    asV1000: [tokenId, memberId, unstakedAmount],
  },
}: EventHandlerContext<'ProjectToken.RevenueSplitLeft'>) {
  const account = await getTokenAccountByMemberByTokenOrFail(overlay, memberId, tokenId)
  account.stakedAmount -= unstakedAmount
  const token = await overlay.getRepository(CreatorToken).getByIdOrFail(tokenId.toString())
  if (token.currentRevenueShareId) {
    // TODO: refactor this as should be true all the times, might be a good idea to panic
    const revenueShare = await overlay
      .getRepository(RevenueShare)
      .getByIdOrFail(token.currentRevenueShareId)
    revenueShare.participantsNum -= 1
    const qRevenueShareParticipation = (
      await overlay
        .getRepository(RevenueShareParticipation)
        .getManyByRelation('accountId', account.id)
    ).find((participation) => participation.recovered === false)
    if (qRevenueShareParticipation) {
      qRevenueShareParticipation.recovered = true
    }
  }
}

I see two problems:

  1. It is possible for a user to unstake tokens/leave revenue share after the creator finalized it. So currentRevenueShareId will be empty at this point (or populated with the next one).
  2. I wonder why we decrease the number of participants, this user participated in the revenue share, leave split event is required to get his tokens back. Plus Atlas uses participantsNum like a counter for a stakers field. To avoid fetching the whole list just to display a number on the revenue share history.
  3. In the light of the possibility of having unrecovered RevenueShareParticipation most of the logic inside the if statement is flawed. Part of it is on the runtime, since now we cannot bind this event to the particular revenue share, just to the token that can have multiple revenue shares. The only thing that comes to my mind is that we could rely on the staked amount and pray that it will change between revenue shares.
zeeshanakram3 commented 7 months ago

Thanks for pointing out the issues

since now we cannot bind this event to the particular revenue share, just to the token that can have multiple revenue shares. The only thing that comes to my mind is that we could rely on the staked amount and pray that it will change between revenue shares.

Even if the token has multiple revenue shares, the User can only participate in at most one revenue split (this is enforced by runtime), so we can actually so we can actually get the RevenueShareParticipation record for given RevenueSplitLeft using the following filter condition

 const revenueShareParticipation = (
    await overlay
      .getRepository(RevenueShareParticipation)
      .getManyByRelation('accountId', account.id)
  ).find((participation) => participation.recovered === false)

commit: 12bbbe2