desktop-wallet icon indicating copy to clipboard operation
desktop-wallet copied to clipboard

add one-click harvesting configuration #1820

Open ishidad2 opened this issue 2 years ago • 4 comments

What the current behaviour is

  • Currently, harvesting in mobile and desktop wallets is working in two steps. #1820

Why this is an issue

  • These two transactions can be combined into one aggregate transaction

What's the fix

  1. Create Account Links (Remote, VRF, Node).
  2. Send a persistent delegated message to Node.

These two transactions are combined into one and sent as an aggregate transaction.

ishidad2 avatar May 23 '22 00:05 ishidad2

Hello @ishidad2 what I noticed:

  1. "Key Links" tab was removed. Could you tell me what is the reason? The goal of this tab was to:
  • allow users to preview what keys are currently set in the account (needed for node operators).
  • allow modifying/removing keys if needed (also set custom keys).
  1. After changes if the account has any key (remote account, VRF, nodes) already set then the user is unable to start harvesting. The wallet is trying to set new keys without unlinking the old one. Error "Validation failed because the main account is already linked to another account" pop-up.

Steps to reproduce: A) Link keys (for example in an old wallet or manually using API) but don't start harvesting. B) Try to start harvesting on the new version wallet.

Example account with linked keys for which I'm not able to start harvesting: https://testnet.symbol.fyi/accounts/TCN2L4FZJ57T5LNKJ635QK26AWIY3X22UDRI6EA

image

Now when you try activate harvesting in wallet after changes:

image image image

Result: Error "Validation failed because the main account is already linked to another account" pop-up. Expected result: It should be possible to start harvesting if we have some keys linked already. The wallet should also:

  • re-use keys if we have private keys in storage. if there are no keys in storage we should generate a new one
  • if we replace keys we should first unlink the old ones. without it request will be rejected

The same error will happen if harvesting will halt on the node side and we will be not able to restart harvesting.

cryptoBeliever avatar May 23 '22 17:05 cryptoBeliever

@cryptoBeliever I was able to reproduce it.

I had forgotten that there was a key-link-only condition in the old wallet, since the new version of the wallet does not generate a key-link-only condition.

I first reverted the "Key Link" tab back to its original state to address this issue.

Also, as another way to approach this I would consider the following Please give me your opinion.

Issue an aggregate transaction with the unlinked transaction also included in the harvest setting.

I believe that doing the above would require a slight change in operations. (Like Arcana wallet, when selecting a node, make an aggregate transaction that includes "LinkAction.Unlink", "LinkAction.Link", and "PersistentDelegationRequestTransaction").

In other words, the "Stop Harvesting" operation should no longer be necessary.

I believe it will take some time to achieve this, as it will require some changes in the UI.

What do you think?

ishidad2 avatar May 23 '22 22:05 ishidad2

@ishidad2 Current logic is very complicated (FormPersistentDelegationRequestTransactionTs.ts) and needs to be analyzed in code (I think many conditions can be removed when we join those two steps) but I think we need actions:

  1. Start harvesting:

Start harvesting should send one aggregate transaction containing:

  • unlink remote account (only if an account is already linked and we don't have a private key for it in storage)
  • unlink VRF key (only if an account is already linked and we don't have a private key for it in storage)
  • unlink node key (if a node key is linked - we need to unlink the old one)
  • link remote account (only if we had to remove the old one - then we have to generate and link a new key)
  • link VRF key (only if we had to remove the old one - then we have to generate and link the new key)
  • link node key (always)
  • persistent delegated message (always)
  1. Stop harvesting:

Stop harvesting should send one aggregate transaction containing:

  • unlink node key (only if a node key is linked do we need to unlink the old one)
  • unlink remote account (only if an account is already linked and we don't have the private key for it in storage)
  • unlink VRF key (only if an account is already linked and we don't have the private key for it in storage)

@yilmazbahadir what do you think about it?

@ishidad2 when we decide on details you can update the issue description to provide more details of what changes introduce in this PR (here examples https://github.com/symbol/desktop-wallet/pull/1847, https://github.com/symbol/product/pull/117 - you can follow template: "What the current behaviour is", "Why this is an issue.", "What's the fix") .

cryptoBeliever avatar May 25 '22 07:05 cryptoBeliever

What the current behaviour is

  • Currently, harvesting in mobile and desktop wallets is working in two steps. #1820

Why this is an issue

  • These two transactions can be combined into one aggregate transaction

What's the fix

  1. Create Account Links (Remote, VRF, Node).
  2. Send a persistent delegated message to Node.

These two transactions are combined into one and sent as an aggregate transaction.

ishidad2 avatar May 26 '22 00:05 ishidad2

Implemented with https://github.com/symbol/desktop-wallet/pull/1903

yilmazbahadir avatar Dec 30 '22 08:12 yilmazbahadir