desktop-wallet
desktop-wallet copied to clipboard
add one-click harvesting configuration #1820
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
- Create Account Links (Remote, VRF, Node).
- Send a persistent delegated message to Node.
These two transactions are combined into one and sent as an aggregate transaction.
Hello @ishidad2 what I noticed:
- "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).
- 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
Now when you try activate harvesting in wallet after changes:
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 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 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:
- 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)
- 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") .
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
- Create Account Links (Remote, VRF, Node).
- Send a persistent delegated message to Node.
These two transactions are combined into one and sent as an aggregate transaction.
Implemented with https://github.com/symbol/desktop-wallet/pull/1903