near / near-wallet

Web wallet for NEAR Protocol which stores keys in browser's localStorage
https://wallet.near.org
MIT License
220 stars 176 forks source link

The rewards earned with staking farming validators are not displayed correctly for the lockup accounts #2430

Open alexauroradev opened 2 years ago

alexauroradev commented 2 years ago

Problem In case an account has a lockup contract then the displayed value of the earned rewards is representing only the main account and not includes the lockup

Expected Behavior Displaying the full rewards OR splitting the lockup rewards and main account rewards

Steps to reproduce

  1. Stake to aurora.pool.near from lockup contract
  2. Observe zero rewards on https://wallet.near.org/staking/aurora.pool.near
stefanopepe commented 2 years ago

We were already looking into it, will add to the backlog with the other changes to the staking page.

I have a parallel question: as a user, how can I withdraw Aurora tokens (or any other fungible token) from a lockup?

stefanopepe commented 2 years ago

@MaximusHaximus do you know if we have any indexer method to look at the fungible tokens balance of lockup contracts?

MaximusHaximus commented 2 years ago

@stefanopepe Since these tokens aren't actually yet 'claimed' from the staking pool, they will not be discovered by our indexer queries because there has not been any interaction between the fungible token contract and the lockup contract. Until the lockup account has claim'd those tokens, we can only query the token farm contract, fetch the un-claimed balance for the lockup account, and then add it into the displayed amount of FTs in the UI. However, this will probably confuse people due to the below concerns I have about how a lockup contract could possibly claim those tokens:

Based on my understanding of lockup accounts, the 'main' account does not generally have an access key that can directly interact with the lockup account, and what the main account is able to do with the lockup account is hard-coded into the lockup account when it is deployed; it does not include things like being able to call ft_transfer or claim on a third-party contract.

If that is true, then:

  1. How will anyone claim rewarded AURORA tokens that were accrued by staking with their lockup account? It’s my understanding that lockup accounts are extremely limited in what they can do.
  2. If I call the claim method on the staking farm as the ‘main’ account, will the staking farm be able to deposit AURORA tokens that were accrued by both the lockup account and the main account into the FT contract under the ‘main’ account id?
  3. If no to (2) above, is it possible to submit a TX that will make the lockup account call methods like claim on the staking farm pool, or ft_transfer on the fungible token contract to transfer the FTs from the lockup account to another account?

If users are unable to control their lockup accounts as described above, then I wonder if we should disable the ability for users to stake with staking farm contracts from a lockup account at all.

In the case of the Aurora staking farm, people are getting 0 NEAR token rewards; if they only receive AURORA tokens as rewards to their lockup contract and they cannot use that lockup contract to claim those rewards, then essentially their AURORA token rewards are trapped on the lockup account unless and until the user is allowed to add an access key to the lockup account, import that account into the wallet, and use it as a primary account to claim the fungible token rewards from the staking farm directly from the lockup account.

For reference, here are the conditions that allow a lockup contract owner to take control of that account by adding a full-access-key: https://github.com/near/core-contracts/blob/master/lockup/src/owner.rs#L503-L507

stefanopepe commented 2 years ago

@alexauroradev I support Daryl's position, we will disable staking of lockup contracts toward new pool.near until we figured out:

@Patrick1904 @corwinharrell would you be interested to share some ideas on the messaging?

MaximusHaximus commented 2 years ago

@alexauroradev Because lockup contracts are immutable, and the 'main' account cannot add any keys to the lockup contracts until the entire balance is vested, we are extremely limited in what we can do from the lockup contract side.

Since users cannot control lockup contracts that are not fully vested (and typically they never actually add a full-access-key to the contract unless they are just about to delete the account entirely and sweep all the funds to another account), what do you think about updating the staking farm contract to understand interactions with lockup contracts?

Ideally:

  1. If the 'owner' account calls the claim method, the staking farm contract would allow the balance being tracked under the associated lockup contract to be claimed to the 'owner' account.
  2. If the 'owner' account calls the get_unclaimed_reward method, the staking farm contract would return a balance that is the sum of the un-claimed rewards for the 'owner' account and the lockup account associated with the owner account.

It occurs to me that if lockup accounts can have unclaimed FT rewards in the farm contract and the farm contract itself is not aware of the lockup 'owner' concept, then we will also need to worry about these balances being orphaned if a lockup account does become fully vested and someone clicks the wallet option to delete the lockup account and sweep all of the funds to their main account.

alexauroradev commented 2 years ago

I believe your understanding on how the staking farm is working at the moment is incorrect. The claim method would move the rewards not into the lockup account, but into the account of the owner of the lockup. CC: @MaximusHaximus @stefanopepe

stefanopepe commented 2 years ago

I believe your understanding on how the staking farm is working at the moment is incorrect. The claim method would move the rewards not into the lockup account, but into the account of the owner of the lockup. CC: @MaximusHaximus @stefanopepe

There's a work in progress to address this specific case, it's included in this pr: #2398