Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. #3555

Unisay · 2022-10-27T08:39:05Z

I did rename Cardano.Wallet.Shelley.Pools to Cardano.Wallet.Pools because Cardano.Wallet.Shelly namespace is a leftover from the times we had 2 separate libraries (core/shelley). It makes sense to me to gradually remove this separation.
split one mkRewardAccountBuilder function into smaller ones (corresponding to individual concers) as it had to many responsibilities at once, was hard to reason about.
removed queryRewardBalance function as it adds no value on top of the getCachedRewardAccountBalance.

(The PR is optimized for reviewing commit-by-commit)

Issue Number

In preparation to ADP-2267

jonathanknowles

Hi @Unisay

Thanks for making this PR!

From looking through, it seems that this PR makes a few distinct changes:

Introduces various readability improvements. (Thanks for this! 💯 )
Introduces the WalletException sum type, which groups together all the individual error types and replaces the use of "errors" with exceptions.
Restructures mkRewardAccountBuilder.
Reworks quitStakePool

Can I trouble you with a very small request: would it be possible to separate this out into separate, smaller PRs? 🙏🏻

Just the readability improvements on their own. (We could approve this very quickly.)
Just the WalletException refactoring on its own. (This deserves more attention and discussion, I feel.)
Just the restructuring of mkRewardAccountBuilder.
The rework of quitStakePool.

In the current PR, the above changes are sometimes mixed together a bit. In particular, the WalletException refactoring is bundled up together with a rework of quitStakePool (in the final commit). But the WalletException work is interesting in its own right (and deserving of its own review, IMO).

Many thanks! Let me know if I can help in any way.

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs

lib/wallet/src/Cardano/Wallet.hs

Unisay · 2022-11-01T14:28:07Z

Can I trouble you with a very small request: would it be possible to separate this out into separate, smaller PRs? 🙏🏻
1. Just the readability improvements on their own. (We could approve this very quickly.)
2. Just the WalletException refactoring on its own. (This deserves more attention and discussion, I feel.)
3. Just the restructuring of mkRewardAccountBuilder.
4. The rework of quitStakePool.

I am afraid that I see the overhead and effort associated with splitting this into individual PRs more vividly than benefits of doing it:
splitting would allow to review/approve/merge only that part independently of other parts; The price for it is: having to spend time and rewrite git history isolating changes; Some of the PRs depend on each other and need to be merged in sequence, etc.

Side note: in practice, such opportunistic-refactoring style modifications happen naturally and aren't well isolated, for example: I started reading code of the quitStakePool, as I read it I also improve it: give better names to expressions (like withdrawal instead of wdrl or walletId instead of wid), I reformat code to improve code style and expression locality. Half way through the process I realize I can simplify function type by getting rid of the ExceptT in favor of exceptions in IO. This triggers a larger change in the way exceptions are handled and defined; Once this is done I am back to modifying quitStakePool.

Anyway, I did my best grouping changes into individual commits (retrospectively), this should facilitate code review.

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs

lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs

lib/wallet/api/http/Cardano/Wallet/Shelley.hs

lib/wallet/src/Cardano/Wallet.hs

Anviking · 2022-11-07T17:02:39Z

bors try

Anviking

Nice! Lgtm.

But it looks like bors try is about to fail

      Migration from Random wallet to target address count: 1. [✘] (1358ms)
      Migration from Random wallet to target address count: 3. [✘] (3828ms)
      Migration from Random wallet to target address count: 10. [✘] (4016ms)
      Migration from Icarus wallet to target address count: 1. [✘] (4177ms)
      Migration from Icarus wallet to target address count: 3. [✘] (4237ms)
      Migration from Icarus wallet to target address count: 10. [✘] (1748ms)

lib/wallet/api/http/Cardano/Wallet/Shelley.hs

lib/wallet/src/Cardano/Wallet.hs

iohk-bors · 2022-11-07T19:46:05Z

try

Build failed:

ci/hydra-build:required

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs

Anviking · 2022-11-08T14:47:31Z

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs

+shelleyOnlyRewardAccountBuilder
    :: forall s k (n :: NetworkDiscriminant)
     . ( HardDerivation k
       , Bounded (Index (AddressIndexDerivationType k) (AddressCredential k))
       , WalletKey k
       , Typeable s
       , Typeable n
       )
-    => Maybe ApiWithdrawalPostData
-    -> Handler (RewardAccountBuilder k)
-mkRewardAccountBuilder withdrawal = do
-    let selfRewardCredentials (rootK, pwdP) =
-            (getRawKey (deriveRewardAccount @k pwdP rootK), pwdP)
+    => ApiWithdrawalPostData
+    -> Either ErrReadRewardAccount (RewardAccountBuilder k)
+shelleyOnlyRewardAccountBuilder w =
    case testEquality (typeRep @s) (typeRep @(SeqState n ShelleyKey)) of
-        Nothing -> liftHandler $ throwE ErrReadRewardAccountNotAShelleyWallet
-        Just Refl -> case withdrawal of
-            Nothing -> pure selfRewardCredentials
-            Just w -> case w of
-                SelfWithdrawal -> pure selfRewardCredentials
-                ExternalWithdrawal (ApiMnemonicT m) -> do
-                    let (xprv, _acct, _path) = W.someRewardAccount @ShelleyKey m
-                    pure (const (xprv, mempty))
+        Nothing -> throwError ErrReadRewardAccountNotAShelleyWallet
+        Just Refl -> case w of
+            SelfWithdrawal -> pure selfRewardAccountBuilder
+            ExternalWithdrawal (ApiMnemonicT m) -> do
+                let (xprv, _acct, _path) = W.someRewardAccount @ShelleyKey m
+                pure (const (xprv, mempty))
+
+selfRewardAccountBuilder
+    :: forall k
+     . ( HardDerivation k
+       , Bounded (Index (AddressIndexDerivationType k) (AddressCredential k))
+       , WalletKey k
+       )
+    => RewardAccountBuilder k
+selfRewardAccountBuilder (rootK, pwdP) =
+    (getRawKey (deriveRewardAccount @k pwdP rootK), pwdP)


Thinking loudly: these are only used from the "old" tx endpoints for the sake of W.buildAndSignTransaction, so we should eventually be able to remove/re-work into some clearer form. I think external withdrawals is what constructTx wouldn't handle automatically, and if it wants to continue being unaware of them, perhaps something like this could work:

postTransactionOld = do tx <- signTx wallet $ addVkWitness externalWithdrawalKey -- after balancing $ balanceTx wallet $ addWithdrawal externalWithdrawal -- before balancing $ constructUnbalancedTx payment ...

(conceptually)

Unisay · 2022-11-08T15:16:43Z

bors r+

3555: Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. r=Unisay a=Unisay - [x] I did [rename Cardano.Wallet.Shelley.Pools to Cardano.Wallet.Pools](3af3732) because `Cardano.Wallet.Shelly` namespace is a leftover from the times we had 2 separate libraries (core/shelley). It makes sense to me to gradually remove this separation. - [x] split one `mkRewardAccountBuilder` function into smaller ones (corresponding to individual concers) as it had to many responsibilities at once, was hard to reason about. - [x] removed `queryRewardBalance` function as it adds no value on top of the `getCachedRewardAccountBalance`. (The PR is optimized for reviewing commit-by-commit) ### Issue Number In preparation to ADP-2267 Co-authored-by: Yura Lazarev <[email protected]>

iohk-bors · 2022-11-08T16:01:16Z

Build failed:

ci/hydra-build:required

Unisay · 2022-11-09T13:48:25Z

bors r+

3555: Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. r=Unisay a=Unisay - [x] I did [rename Cardano.Wallet.Shelley.Pools to Cardano.Wallet.Pools](3af3732) because `Cardano.Wallet.Shelly` namespace is a leftover from the times we had 2 separate libraries (core/shelley). It makes sense to me to gradually remove this separation. - [x] split one `mkRewardAccountBuilder` function into smaller ones (corresponding to individual concers) as it had to many responsibilities at once, was hard to reason about. - [x] removed `queryRewardBalance` function as it adds no value on top of the `getCachedRewardAccountBalance`. (The PR is optimized for reviewing commit-by-commit) ### Issue Number In preparation to ADP-2267 Co-authored-by: Yura Lazarev <[email protected]>

iohk-bors · 2022-11-09T13:49:43Z

Build failed:

buildkite/cardano-wallet

Unisay · 2022-11-09T14:20:44Z

bors retry

Unisay · 2022-11-11T18:00:59Z

bors r+

3555: Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. r=Unisay a=Unisay - [x] I did [rename Cardano.Wallet.Shelley.Pools to Cardano.Wallet.Pools](3af3732) because `Cardano.Wallet.Shelly` namespace is a leftover from the times we had 2 separate libraries (core/shelley). It makes sense to me to gradually remove this separation. - [x] split one `mkRewardAccountBuilder` function into smaller ones (corresponding to individual concers) as it had to many responsibilities at once, was hard to reason about. - [x] removed `queryRewardBalance` function as it adds no value on top of the `getCachedRewardAccountBalance`. (The PR is optimized for reviewing commit-by-commit) ### Issue Number In preparation to ADP-2267 Co-authored-by: Yura Lazarev <[email protected]>

iohk-bors · 2022-11-11T18:07:16Z

Build failed:

ci/hydra-build:required

Unisay · 2022-11-12T07:10:07Z

bors merge

3555: Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. r=Unisay a=Unisay - [x] I did [rename Cardano.Wallet.Shelley.Pools to Cardano.Wallet.Pools](3af3732) because `Cardano.Wallet.Shelly` namespace is a leftover from the times we had 2 separate libraries (core/shelley). It makes sense to me to gradually remove this separation. - [x] split one `mkRewardAccountBuilder` function into smaller ones (corresponding to individual concers) as it had to many responsibilities at once, was hard to reason about. - [x] removed `queryRewardBalance` function as it adds no value on top of the `getCachedRewardAccountBalance`. (The PR is optimized for reviewing commit-by-commit) ### Issue Number In preparation to ADP-2267 Co-authored-by: Yura Lazarev <[email protected]>

iohk-bors · 2022-11-12T07:16:53Z

Build failed:

ci/hydra-build:required

Unisay · 2022-11-14T08:42:48Z

bors merge

3555: Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. r=Unisay a=Unisay - [x] I did [rename Cardano.Wallet.Shelley.Pools to Cardano.Wallet.Pools](3af3732) because `Cardano.Wallet.Shelly` namespace is a leftover from the times we had 2 separate libraries (core/shelley). It makes sense to me to gradually remove this separation. - [x] split one `mkRewardAccountBuilder` function into smaller ones (corresponding to individual concers) as it had to many responsibilities at once, was hard to reason about. - [x] removed `queryRewardBalance` function as it adds no value on top of the `getCachedRewardAccountBalance`. (The PR is optimized for reviewing commit-by-commit) ### Issue Number In preparation to ADP-2267 Co-authored-by: Yura Lazarev <[email protected]>

iohk-bors · 2022-11-14T09:06:48Z

Build failed:

ci/hydra-build:required

Anviking · 2022-11-14T09:59:52Z

bors r+

iohk-bors · 2022-11-14T13:59:25Z

Build succeeded:

…saction` within `quitStakePool`. r=Anviking a=Unisay - [x] I did [rename Cardano.Wallet.Shelley.Pools to Cardano.Wallet.Pools](3af3732) because `Cardano.Wallet.Shelly` namespace is a leftover from the times we had 2 separate libraries (core/shelley). It makes sense to me to gradually remove this separation. - [x] split one `mkRewardAccountBuilder` function into smaller ones (corresponding to individual concers) as it had to many responsibilities at once, was hard to reason about. - [x] removed `queryRewardBalance` function as it adds no value on top of the `getCachedRewardAccountBalance`. (The PR is optimized for reviewing commit-by-commit) ### Issue Number In preparation to ADP-2267 Co-authored-by: Yura Lazarev <[email protected]> Source commit: a3e6478

3583: Revise error name to `WithdrawalNotBeneficial`. r=jonathanknowles a=jonathanknowles ## Issue Number None. Noticed while reviewing #3555. ## Description This PR renames `ErrWithdrawalNotWorth` to `ErrWithdrawalNotBeneficial`. The wording of the former error name is broken. We _can_ say "a withdrawal is not worth **it**", but saying something is "not worth" (without the "it") just doesn't make sense in English. ## Compatibility Usages of the JSON error code `withdrawal_not_worth` are very rare: [Usages of `withdrawal_not_worth`](https://github.com/search?q=org%3Ainput-output-hk+%22withdrawal_not_worth%22&type=code). These rare cases can easily be updated with a tiny PR or two. Co-authored-by: Jonathan Knowles <[email protected]>

…`. r=jonathanknowles a=jonathanknowles ## Issue Number None. Noticed while reviewing #3555. ## Description This PR renames `ErrWithdrawalNotWorth` to `ErrWithdrawalNotBeneficial`. The wording of the former error name is broken. We _can_ say "a withdrawal is not worth **it**", but saying something is "not worth" (without the "it") just doesn't make sense in English. ## Compatibility Usages of the JSON error code `withdrawal_not_worth` are very rare: [Usages of `withdrawal_not_worth`](https://github.com/search?q=org%3Ainput-output-hk+%22withdrawal_not_worth%22&type=code). These rare cases can easily be updated with a tiny PR or two. Co-authored-by: Jonathan Knowles <[email protected]> Source commit: d59b05e

Unisay requested review from Anviking and HeinrichApfelmus October 27, 2022 08:39

HeinrichApfelmus requested a review from jonathanknowles October 27, 2022 09:58

Unisay force-pushed the yura/ADP-2267/quit-sp-balance-tx branch 2 times, most recently from 33eecf8 to c5c3b38 Compare October 31, 2022 16:35

jonathanknowles reviewed Nov 1, 2022

View reviewed changes

lib/wallet/api/http/Cardano/Wallet/Api/Http/Shelley/Server.hs Outdated Show resolved Hide resolved

lib/wallet/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved

Unisay force-pushed the yura/ADP-2267/quit-sp-balance-tx branch from c5c3b38 to 4bfe080 Compare November 1, 2022 11:51

Unisay force-pushed the yura/ADP-2267/quit-sp-balance-tx branch from d3af78c to a51009d Compare November 2, 2022 15:45

Anviking reviewed Nov 2, 2022

View reviewed changes

Unisay force-pushed the yura/ADP-2267/quit-sp-balance-tx branch from 19f689c to 03bfe2c Compare November 7, 2022 12:01

Unisay requested a review from Anviking November 7, 2022 13:27

Anviking requested changes Nov 7, 2022

View reviewed changes

lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs Outdated Show resolved Hide resolved

lib/wallet/api/http/Cardano/Wallet/Shelley.hs Outdated Show resolved Hide resolved

lib/wallet/src/Cardano/Wallet.hs Show resolved Hide resolved

iohk-bors bot added a commit that referenced this pull request Nov 7, 2022

Try #3555:

7a5d805

Anviking reviewed Nov 7, 2022

View reviewed changes

lib/wallet/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved

Unisay force-pushed the yura/ADP-2267/quit-sp-balance-tx branch 2 times, most recently from d677889 to e552b8c Compare November 8, 2022 13:27

Unisay requested a review from Anviking November 8, 2022 14:20

Anviking approved these changes Nov 8, 2022

View reviewed changes

Unisay force-pushed the yura/ADP-2267/quit-sp-balance-tx branch from e552b8c to b08c8be Compare November 9, 2022 11:53

Unisay added 11 commits November 11, 2022 18:09

WalletException gets converted to ServerError in hoistServer

2916b9c

refactor: rename c to coin

7a083a4

WalletException gets converted to ServerError in hoistServer

2620043

Replace throw with throwIO

5848c76

chore: reorder imports

239d8fc

refactor: simplify handleWalletExceptions

fe4adf5

Split mkRewardAccoundBuilder into smaller builders

bec7ecc

chore: reorder import

7cdc038

fix: zero reward is not worth withdrawing

b1f3300

self withdrawal won't throw if its not worth

c072c2b

fixup! remove unnecessary polymorphism

eee2269

Unisay force-pushed the yura/ADP-2267/quit-sp-balance-tx branch from a8a2027 to eee2269 Compare November 11, 2022 17:09

Touch CI

6116528

iohk-bors bot merged commit a3e6478 into master Nov 14, 2022

iohk-bors bot deleted the yura/ADP-2267/quit-sp-balance-tx branch November 14, 2022 13:59

jonathanknowles mentioned this pull request Nov 14, 2022

Revise error name to WithdrawalNotBeneficial. #3583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. #3555

Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. #3555

Unisay commented Oct 27, 2022

jonathanknowles left a comment

Unisay commented Nov 1, 2022

Anviking commented Nov 7, 2022

Anviking left a comment

iohk-bors bot commented Nov 7, 2022

Anviking Nov 8, 2022

Unisay commented Nov 8, 2022

iohk-bors bot commented Nov 8, 2022

Unisay commented Nov 9, 2022

iohk-bors bot commented Nov 9, 2022

Unisay commented Nov 9, 2022

Unisay commented Nov 11, 2022

iohk-bors bot commented Nov 11, 2022

Unisay commented Nov 12, 2022

iohk-bors bot commented Nov 12, 2022

Unisay commented Nov 14, 2022

iohk-bors bot commented Nov 14, 2022

Anviking commented Nov 14, 2022

iohk-bors bot commented Nov 14, 2022

Refactor in preparation to reuse balanceTransaction within quitStakePool. #3555

Refactor in preparation to reuse balanceTransaction within quitStakePool. #3555

Conversation

Unisay commented Oct 27, 2022

Issue Number

jonathanknowles left a comment

Choose a reason for hiding this comment

Unisay commented Nov 1, 2022

Anviking commented Nov 7, 2022

Anviking left a comment

Choose a reason for hiding this comment

iohk-bors bot commented Nov 7, 2022

try

Anviking Nov 8, 2022

Choose a reason for hiding this comment

Unisay commented Nov 8, 2022

iohk-bors bot commented Nov 8, 2022

Unisay commented Nov 9, 2022

iohk-bors bot commented Nov 9, 2022

Unisay commented Nov 9, 2022

Unisay commented Nov 11, 2022

iohk-bors bot commented Nov 11, 2022

Unisay commented Nov 12, 2022

iohk-bors bot commented Nov 12, 2022

Unisay commented Nov 14, 2022

iohk-bors bot commented Nov 14, 2022

Anviking commented Nov 14, 2022

iohk-bors bot commented Nov 14, 2022

Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. #3555

Refactor in preparation to reuse `balanceTransaction` within `quitStakePool`. #3555