Withdrawals in multisig #3925

paweljakubas · 2023-05-11T15:35:01Z

Added withdrawal support for multisig
Added withdrawal integration test
Added quit integration test
Make sure getTransaction exposes certificates - shown in integration testing

Comments

Issue Number

piotr-iohk · 2023-05-23T13:05:20Z

lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Transactions.hs

+                >>= flip verify
+                    [ expectField
+                        (#balance . #reward)
+                        (`shouldBe` (Quantity 0))


We should probably add a check that we have some rewards before withdrawing them.

This is checked before withdrawal and after withdrawal (should be zero as you underlined above):

1986 eventually "party1: Wallet gets rewards from pool1" $ do 1987 r <- request @ApiWallet ctx (Link.getWallet @'Shared party1) Default Empty 1988 verify r 1989 [ expectField 1990 (#balance . #reward) 1991 (.> (Quantity 0)) 1992 ] 1993 eventually "party2: Wallet gets rewards from pool1" $ do 1994 r <- request @ApiWallet ctx (Link.getWallet @'Shared party2) Default Empty 1995 verify r 1996 [ expectField 1997 (#balance . #reward) 1998 (.> (Quantity 0)) 1999 ]

piotr-iohk · 2023-05-23T13:22:46Z

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

+                [ "I couldn't read a reward account which is required for "
+                , "withdrawals. Either there is db malfunction or withdrawals "
+                , "was used for shared wallets missing delegation template."


Suggested change

[ "I couldn't read a reward account which is required for "

, "withdrawals. Either there is db malfunction or withdrawals "

, "was used for shared wallets missing delegation template."

[ "Unable to read the reward account required for withdrawals. "

, "It appears that the withdrawals feature was utilized for a shared wallet "

, "without the corresponding delegation template."

piotr-iohk · 2023-05-23T13:23:50Z

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

+            [ "It is regrettable but you've just attempted an operation "
+            , "that is invalid for this type of wallet. Only new 'Shelley' and "
+            , "'Shared' wallets can do something with rewards and this one isn't."
+            ]


Suggested change

[ "It is regrettable but you've just attempted an operation "

, "that is invalid for this type of wallet. Only new 'Shelley' and "

, "'Shared' wallets can do something with rewards and this one isn't."

]

[ "It is regrettable but you've just attempted an operation "

, "that is invalid for this type of wallet. Only new 'Shelley' and "

, "'Shared' wallets have the capability to perform actions with rewards, "

, "which is not applicable to the current wallet."

]

paweljakubas · 2023-05-23T14:30:18Z

bors try

iohk-bors · 2023-05-23T14:55:44Z

try

Build failed:

buildkite/cardano-wallet

paweljakubas · 2023-05-24T10:34:46Z

bors try

iohk-bors · 2023-05-24T11:08:27Z

try

Build succeeded:

buildkite/cardano-wallet

Anviking · 2023-05-24T15:20:39Z

lib/wallet/src/Cardano/Wallet.hs

+    when (isNothing rewardAccountM) $
+        throwIO $ ExceptionReadRewardAccount ErrReadRewardAccountMissing


I wonder if we can unify the shared vs non-shared branches via:

readRewardAccount :: forall s. Including '[ShelleyKey, SharedKey] (KeyOf s) => DBLayer IO s -> IO (RewardAccount, XPub, NonEmpty DerivationIndex) readRewardAccount = do ... case keyFlavor @(KeyOf s) of ShelleyKeyS -> ... SharedKeyS -> ... case ... of Just _ -> ... Nothing -> throwIO $ ExceptionReadRewardAccount ErrReadRewardAccountMissing

It does at least look like all callers of readSharedRewardAccount are throwing in the Nothing case anyway... Though hiding the Nothing more would be deceptive.

I suppose it would alternatively be possible to factor out the reading of the reward account somehow.

good sugestion. done

Anviking · 2023-05-24T15:29:43Z

lib/wallet/src/Cardano/Wallet.hs

+
+sharedOnlyReadRewardAccount
+    :: forall s
+     . WalletFlavor s


Why not enforce that this is a shared wallet with WalletFlavor s ~ SharedWallet or something similar?

I followed the shelleyOnlyReadRewardAccount pattern here. So want unsafe version that emits error if used not with non-shared wallet.

Anviking · 2023-05-25T13:49:46Z

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

+        ErrReadRewardAccountMissing ->
+            apiError err501 MissingRewardAccount $ mconcat


👌 ErrConstructTxDelegationInvalid is what users would see

Anviking · 2023-05-25T13:52:31Z

lib/wallet/src/Cardano/Wallet.hs

+    let (rewardAccount, derivationPath) = fromJust rewardAccountM
+    balance <- getCachedRewardAccountBalance netLayer rewardAccount
+    pp <- currentProtocolParameters netLayer
+    return $ case checkRewardIsWorthTxCost txWitnessTag pp era balance of


It seems checkRewardIsWorthTxCost will underestimate the cost of making a withdrawal as it doesn't take the size of the delegation script into account.

I suppose you could

A

Pass in the delegationTemplate checkRewardIsWorthTxCost. We'd then also need to update the logic here https://github.com/input-output-hk/cardano-wallet/blob/ca7eba61eb11b576d6e20a2237955c064ecd2b7a/lib/wallet/src/Cardano/Wallet/Shelley/Transaction.hs#L1774 which would be antithetical to having estimateTxSize know the bare minimum to support coin-selection.

B

Define

-- | Upper estimation of the number of bytes needed for a withdrawal. txRewardWithdrawalSize :: WalletState s -> TxSize

which would be more in line with https://input-output.atlassian.net/browse/ADP-3016

C

Do nothing, or raise hard-code limit of 2 * costOfWithdrawal.

This would likely work fine for most users. Users with large delegation templates could lose money from withdrawals which are too small to be worth it. Although I believe it would be on the order of 0.01 ada, and at most once per epoch... So probably completely fine...

opted for A

Unless you also update the defaultTransactionCtx in checkRewardIsWorthTxCost, your changes have no effect

https://github.com/input-output-hk/cardano-wallet/blob/f1757d4a398bc200930b316d64f82ec4e4a44a42/lib/wallet/src/Cardano/Wallet.hs#L1287-L1288

Anviking · 2023-05-25T14:53:14Z

lib/wallet/src/Cardano/Wallet.hs

+    rewardAccountM <- readSharedRewardAccount db
+    when (isNothing rewardAccountM) $
+        throwIO $ ExceptionReadRewardAccount ErrReadRewardAccountMissing
+    let (rewardAccount, derivationPath) = fromJust rewardAccountM


Again no need for fromJust, they are a bad sign, and here it's completely avoidable.

thanks to unifying readRewardAccount not valid anymore

Anviking · 2023-05-25T15:06:50Z

lib/wallet/src/Cardano/Wallet/Shelley/Transaction.hs

+                        if ourRewardAcctM == Just rewardAcct then
+                            constructUnsignedTx networkId (md, []) ttl wdrl
+                            selection delta assetsToBeMinted assetsToBeBurned inpsScripts
+                            stakingScriptM
+                            (Write.shelleyBasedEraFromRecentEra Write.recentEra)
+                        else
+                            constructUnsignedTx networkId (md, []) ttl wdrl
+                            selection delta assetsToBeMinted assetsToBeBurned inpsScripts
+                            Nothing
+                            (Write.shelleyBasedEraFromRecentEra Write.recentEra)


I suspect these branches (including _ ->) and the case in mkUnsignedTx could be unified through some one

mkWithdrawal :: wallet flavor or delegationTemplate etc -> Withdrawal -> Cardano.TxWithdrawals build era

rather than having logic spread out.

Could you think about it / have look on how to reduce the complexity here? 🙏

(There seem to be so many "layers" in this module (mkUnsignedTx, mkUnsignedTransaction, constructUnsignedTransaction etc), we should move towards flattening them)

I tried to use pattern matching but in the end it turn out not to be much easier .... I agree we need to unify and cut those artificial layers - but I would write down separate ticket for that - I am happy to take it.

Thanks, please do if you see a plan of attack

paweljakubas · 2023-05-29T15:41:32Z

bors try

iohk-bors · 2023-05-29T15:45:45Z

try

Build failed:

buildkite/cardano-wallet

paweljakubas · 2023-05-29T17:06:16Z

bors try

iohk-bors · 2023-05-29T17:39:12Z

try

Build failed:

buildkite/cardano-wallet

rm debugs

paweljakubas · 2023-05-31T10:01:19Z

bors r+

iohk-bors · 2023-05-31T10:44:03Z

Build succeeded:

buildkite/cardano-wallet

…a=paweljakubas  - [x] Added withdrawal support for multisig - [x] Added withdrawal integration test - [x] Added quit integration test - [x] Make sure getTransaction exposes certificates - shown in integration testing ### Comments  ### Issue Number adp-2604  3970: Derive arithmetic operations for `{Coin,TokenQuantity}`. r=jonathanknowles a=jonathanknowles ## Summary This PR uses classes defined by the [`monoid-subclasses`](https://hackage.haskell.org/package/monoid-subclasses-1.2.3#readme) library to derive the definitions of all arithmetic operations for the `Coin` and `TokenQuantity` types. This takes advantage of the fact that both `Coin` and `TokenQuantity` have `Semigroup` and `Monoid` instances that are identical to those for `Sum Natural`. Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Piotr Stachyra <[email protected]> Co-authored-by: Jonathan Knowles <[email protected]> Source commit: 8622cfd

paweljakubas self-assigned this May 11, 2023

paweljakubas mentioned this pull request May 11, 2023

Paweljakubas/tmp #3916

Closed

1 task

paweljakubas force-pushed the paweljakubas/adp-2604/withdrawals-in-multisig branch 3 times, most recently from d7d4636 to d21e220 Compare May 23, 2023 12:44

paweljakubas requested review from Anviking and piotr-iohk May 23, 2023 12:50

paweljakubas marked this pull request as ready for review May 23, 2023 12:50

piotr-iohk reviewed May 23, 2023

View reviewed changes

iohk-bors bot added a commit that referenced this pull request May 23, 2023

Try #3925:

3b7460e

paweljakubas force-pushed the paweljakubas/adp-2604/withdrawals-in-multisig branch from 0784414 to c55c61c Compare May 24, 2023 10:34

iohk-bors bot added a commit that referenced this pull request May 24, 2023

Try #3925:

0000c17

paweljakubas requested a review from piotr-iohk May 24, 2023 11:26

Anviking reviewed May 25, 2023

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-2604/withdrawals-in-multisig branch from ca7eba6 to f0561b9 Compare May 29, 2023 15:39

iohk-bors bot added a commit that referenced this pull request May 29, 2023

Try #3925:

07889e8

paweljakubas requested a review from Anviking May 29, 2023 17:06

iohk-bors bot added a commit that referenced this pull request May 29, 2023

Try #3925:

b9fc8c7

paweljakubas force-pushed the paweljakubas/adp-2604/withdrawals-in-multisig branch from 9f4cfc7 to 2f5106f Compare May 30, 2023 09:23

paweljakubas and others added 24 commits May 31, 2023 11:59

extend integration testing 1

62aa41f

extend integration testing 2

f6e42e9

call proper mkWihtdrawal from Cardano.Wallet

57e7612

deal with withdrawals in decodeSharedTx

04a2caf

post-rebase compilation fix

4a9d56e

rm debugs

deal with withdrawal script wits

fd065f1

extend integration test

b848372

quit in integration test - part 1

2206695

quit in integration test - part 2

47a4019

quit in integration test - part 3

202de20

quit in integration test - part 4

e728db2

add failing expectation for getTransaction

d975397

make cert visible in getTransaction for multisig

4608dfb

more getTransaction testing

5c503b3

more getTransaction testing - part 2

571bbc3

small cleanups

4168fda

refactor InvalidWalletType error

bc96cf9

e2e tests: update certificates visibility on deleg tx

1e9df25

refactor readRewardAccount and use it also for shared wallets

9845dee

fix size estimation due to optional script withdrawals

1a608e8

hlint

d895589

deal properly with withdrawals after refactoring

6604359

post rebase fix

b5f631d

bolster checkRewardIsWorthTxCost

16f0cb4

paweljakubas force-pushed the paweljakubas/adp-2604/withdrawals-in-multisig branch from 419638e to 16f0cb4 Compare May 31, 2023 10:00

iohk-bors bot merged commit 8622cfd into master May 31, 2023

iohk-bors bot deleted the paweljakubas/adp-2604/withdrawals-in-multisig branch May 31, 2023 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Withdrawals in multisig #3925

Withdrawals in multisig #3925

paweljakubas commented May 11, 2023 •

edited

Loading

piotr-iohk May 23, 2023

paweljakubas May 23, 2023 •

edited

Loading

piotr-iohk May 23, 2023

paweljakubas May 23, 2023

piotr-iohk May 23, 2023

paweljakubas May 23, 2023

paweljakubas commented May 23, 2023

iohk-bors bot commented May 23, 2023

paweljakubas commented May 24, 2023

iohk-bors bot commented May 24, 2023

Anviking May 24, 2023

paweljakubas May 29, 2023

Anviking May 24, 2023

paweljakubas May 29, 2023

Anviking May 25, 2023

Anviking May 25, 2023

Anviking May 25, 2023

paweljakubas May 29, 2023

Anviking May 30, 2023

paweljakubas May 31, 2023

Anviking May 25, 2023

paweljakubas May 29, 2023

Anviking May 25, 2023 •

edited

Loading

Anviking May 25, 2023

paweljakubas May 29, 2023

Anviking May 30, 2023

paweljakubas commented May 29, 2023

iohk-bors bot commented May 29, 2023

paweljakubas commented May 29, 2023

iohk-bors bot commented May 29, 2023

paweljakubas commented May 31, 2023

iohk-bors bot commented May 31, 2023

		when (isNothing rewardAccountM) $
		throwIO $ ExceptionReadRewardAccount ErrReadRewardAccountMissing

		ErrReadRewardAccountMissing ->
		apiError err501 MissingRewardAccount $ mconcat

Withdrawals in multisig #3925

Withdrawals in multisig #3925

Conversation

paweljakubas commented May 11, 2023 • edited Loading

Comments

Issue Number

Choose a reason for hiding this comment

paweljakubas May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented May 23, 2023

iohk-bors bot commented May 23, 2023

try

paweljakubas commented May 24, 2023

iohk-bors bot commented May 24, 2023

try

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A

B

C

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented May 29, 2023

iohk-bors bot commented May 29, 2023

try

paweljakubas commented May 29, 2023

iohk-bors bot commented May 29, 2023

try

paweljakubas commented May 31, 2023

iohk-bors bot commented May 31, 2023

paweljakubas commented May 11, 2023 •

edited

Loading

paweljakubas May 23, 2023 •

edited

Loading

Anviking May 25, 2023 •

edited

Loading