Rewards in shared wallets #3842

paweljakubas · 2023-04-07T14:07:05Z

Extending shared state
add readRewardAccount
add manageSharedBalance
use manageSharedBalance
impl IsOurs for RewardAccount
extending integration testing (joining/rejoining)

Comments

PR on top of #3830

Issue Number

adp-2603

cdn

cleans

cleanings

more WitCountCtx

jonathanknowles

Hi @paweljakubas

Just some very early initial comments and suggestions!

jonathanknowles · 2023-04-11T07:37:50Z

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

+        let rewardAcctM = case scriptM of
+                Just script ->
+                    let scriptHash =
+                            CA.unScriptHash $
+                            CA.toScriptHash script
+                    in Just $ RewardAccount scriptHash
+                Nothing -> Nothing


I think we can rewrite this as:

Suggested change

let rewardAcctM = case scriptM of

Just script ->

let scriptHash =

CA.unScriptHash $

CA.toScriptHash script

in Just $ RewardAccount scriptHash

Nothing -> Nothing

let rewardAcctM =

RewardAccount . CA.unScriptHash . CA.toScriptHash <$> scriptM

of course, done

jonathanknowles · 2023-04-11T07:43:55Z

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

+                    if numberStakingNativeScripts == 0 then
+                        0
+                    else if numberStakingNativeScripts == 1 then
+                        Shared.estimateMinWitnessRequiredPerInput scriptD
+                    else
+                        error "wallet supports transactions with 0 or 1 staking script"


Can this be rewritten with pattern guards or MultiwayIf?

Something like:

Suggested change

if numberStakingNativeScripts == 0 then

0

else if numberStakingNativeScripts == 1 then

Shared.estimateMinWitnessRequiredPerInput scriptD

else

error "wallet supports transactions with 0 or 1 staking script"

| numberStakingNativeScripts == 0 =

0

| numberStakingNativeScripts == 1 =

Shared.estimateMinWitnessRequiredPerInput scriptD

| otherwise =

error "wallet supports transactions with 0 or 1 staking script"

jonathanknowles · 2023-04-11T07:47:20Z

lib/wallet/src/Cardano/Wallet.hs

+    walletState <- getState <$>
+        withExceptT ErrReadRewardAccountNoSuchWallet
+            (readWalletCheckpoint db wid)
+    pure $ Shared.rewardAccountKey walletState


Suggested change

walletState <- getState <$>

withExceptT ErrReadRewardAccountNoSuchWallet

(readWalletCheckpoint db wid)

pure $ Shared.rewardAccountKey walletState

Shared.rewardAccountKey . getState <$>

withExceptT ErrReadRewardAccountNoSuchWallet

(readWalletCheckpoint db wid)

it i not in this form anymore. path was added

jonathanknowles · 2023-04-11T07:47:42Z

lib/wallet/src/Cardano/Wallet.hs

+    readWalletCheckpoint ::
+        DBLayer IO s k -> WalletId -> ExceptT ErrNoSuchWallet IO (Wallet s)


Suggested change

readWalletCheckpoint ::

DBLayer IO s k -> WalletId -> ExceptT ErrNoSuchWallet IO (Wallet s)

readWalletCheckpoint

:: DBLayer IO s k -> WalletId -> ExceptT ErrNoSuchWallet IO (Wallet s)

jonathanknowles · 2023-04-11T07:49:34Z

lib/wallet/src/Cardano/Wallet.hs

+        stakingKeyM = case xprvM of
+            Just xprv -> Just
+                ( hashVerificationKey @k CA.Delegation $ liftRawKey $ toXPub xprv
+                , xprv
+                , rootPwd
+                )
+            Nothing -> Nothing


Suggested change

stakingKeyM = case xprvM of

Just xprv -> Just

( hashVerificationKey @k CA.Delegation $ liftRawKey $ toXPub xprv

, xprv

, rootPwd

)

Nothing -> Nothing

stakingKeyM = xprvM <&> \xprv ->

( hashVerificationKey @k CA.Delegation $ liftRawKey $ toXPub xprv

, xprv

, rootPwd

)

neat form! done

jonathanknowles · 2023-04-11T07:53:29Z

lib/wallet/src/Cardano/Wallet/Shelley/Network/Node.hs

+                Left rewardAcct ->
+                    _getCachedRewardAccountBalance rewardsObserverFromKeyHash rewardAcct
+                Right rewardAcct ->
+                    _getCachedRewardAccountBalance rewardsObserverFromScriptHash rewardAcct


Suggested change

_getCachedRewardAccountBalance rewardsObserverFromScriptHash rewardAcct

_getCachedRewardAccountBalance

rewardsObserverFromScriptHash rewardAcct

jonathanknowles · 2023-04-11T07:55:28Z

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

@@ -453,6 +453,11 @@ instance IsServerError ErrConstructTx where
            , "'PATCH /shared-wallets/{walletId}/delegation-script-template'"
            , "to make it suitable for constructing transactions."
            ]
+        ErrConstructTxStakingInvalid ->
+            apiError err403 StakingInvalid $ T.unwords
+            [ "I cannot construct a delegating transaction for a shared wallet "


Suggested change

[ "I cannot construct a delegating transaction for a shared wallet "

[ "I cannot construct a delegating transaction for a shared wallet"

jonathanknowles · 2023-04-11T07:56:34Z

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

+            apiError err404 MissingRewardAccount $ mconcat
+                [ "I couldn't read a reward account which is required for reward "
+                , "detection. Either there is db malfunction or managing rewards "
+                , "was used for shared wallets missing delegation template."


Hi @paweljakubas

Could you explain a little more about what this means:
"managing rewards was used for shared wallets missing delegation template."

shared wallets can be with and without delegation template. When the delegation template is missing we are having wallet that cannot stake, is generating enterprise addresses (only with payment credential). In that case we should not spin up "managing awards". It is the error that make sure "managing awards service" is used for multisig wallets with delegation templates present

jonathanknowles · 2023-04-11T08:01:24Z

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

-        , depositsReturned = []
+        , certificates = certs
+        , depositsTaken =
+            (Quantity . fromIntegral . unCoin . W.stakeKeyDeposit $ pp)


Suggestion: use Coin.toQuantity (this is a statically-checked total function, whereas fromIntegral is statically-unchecked and partial).

Suggested change

(Quantity . fromIntegral . unCoin . W.stakeKeyDeposit $ pp)

(Coin.toQuantity . W.stakeKeyDeposit $ pp)

good suggestion. done

jonathanknowles · 2023-04-11T08:02:15Z

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

+            (Quantity . fromIntegral . unCoin . W.stakeKeyDeposit $ pp)
+                <$ filter ourRewardAccountRegistration certs
+        , depositsReturned =
+            (Quantity . fromIntegral . unCoin . W.stakeKeyDeposit $ pp)


Suggestion: use Coin.toQuantity (this is a statically-checked total function, whereas fromIntegral is statically-unchecked and partial).

Suggested change

(Quantity . fromIntegral . unCoin . W.stakeKeyDeposit $ pp)

(Coin.toQuantity . W.stakeKeyDeposit $ pp)

Anviking · 2023-04-12T15:01:58Z

lib/wallet/src/Cardano/Wallet/Network.hs

@@ -160,7 +160,8 @@ data NetworkLayer m block = NetworkLayer
        -> m StakePoolsSummary

    , getCachedRewardAccountBalance
-        :: RewardAccount
+        :: Either RewardAccount RewardAccount


Is the distinction between RewardAccount and RewardAccount really needed, or could we not get away with just RewardAccount?

yes, one is key hashes based and another is script hashes based. And this information is need when transforming from node reward account. See how this point is used in Cardano.Wallet.Shelley.Network.Node module (shelleyQry)

Ah, so I take it really is a binary tag for script vs keyHash object we need for the query, and is not already in the RewardAccount bytes...

Anviking · 2023-04-17T11:39:38Z

lib/wallet/src/Cardano/Wallet/Shelley/Network/Node.hs

+    rewardsObserverFromKeyHash <- newRewardBalanceFetcher
+        tr readNodeTip queryRewardQ RewardAccountFromKeyHash
+    rewardsObserverFromScriptHash <- newRewardBalanceFetcher
+        tr readNodeTip queryRewardQ RewardAccountFromScriptHash


Creating two different fetchers with two different RewardAccountSource is unnecessary. Batching all script and key credentials into the same local state query should be more efficient. The reason you can't currently do this is the RewardAccount type containing too little information.

What about:

Either importing or re-exporting Cardano.StakeCredential / Ledger.StakeCredential crypto or defining something similar to it. (Could also be data StakeCredential = ...FromKeyHash RewardAccount | ...FromScriptHash RewardAccount)

Changing getCachedRewardAccountBalance to take StakeCredential instead of Either RewardAccount RewardAccount

Removing RewardAccountSource and the duplicate call to newRewardBalanceFetcher

?

Anviking

#3842 (comment)

paweljakubas · 2023-04-20T10:08:42Z

closing in favor of #3872

paweljakubas added 28 commits April 6, 2023 09:36

add delegationAction to constructSharedTransaction

f4e57d6

slimming constraints in constructSharedTransaction

358e351

introduce ErrConstructTxStakingInvalid

f84479f

integration test for ErrConstructTxStakingInvalid

ec05547

extend mkUnsignedTx

76ed1b3

cdn

add emptySharedWalletDelegating and fixtureSharedWalletdelegating

1f78538

add constructTx integration test

7a0b094

sneak certs to tx

a853ad1

deal with refund/deposit

956c73b

certs support in decodeSharedTransaction - part 1

bd8e0b8

cleans

update certs in decodeSharedTransaction

ac18c0d

use certs in integration testing

e0efab4

better code reuse

b5b5e83

fix SharedWitnessCtx

9c3a11e

cleanings

sign shared tx with delegation -part 1

19efab8

sign shared tx with delegation -part 2

ee5583e

sign shared tx with delegation - part 3

a281359

sign shared tx with delegation - part 4

d95c06b

cleanings

add rest integration test

1f157d0

introduce ToWitnessCountCtx

f778b03

more WitCountCtx

boost submitSharedTransaction

5e7f7e4

hlint

18c509b

proving wallet is delegating from getTransaction

6718bfe

add integration detecting rewards

1f12280

add rewardAcct to SharedState

237b3d3

add readSharedRewardAccount

f288324

add manageSharedRewardBalance

d8c1efe

use manageSharedBalance in Server

f5c83fb

paweljakubas self-assigned this Apr 7, 2023

introduce toScriptHashStakeCredential

f6202d8

paweljakubas added 2 commits April 7, 2023 18:53

enhancements

d5a7304

isOurs for RewardAccount

60febbd

jonathanknowles reviewed Apr 11, 2023

View reviewed changes

paweljakubas added 2 commits April 11, 2023 13:54

expand on integration test - rejoin

fa9e7fe

add rest expectations for rejoin

50e5a93

paweljakubas changed the base branch from master to paweljakubas/adp-2602/shared-wallets-delegation April 11, 2023 16:45

paweljakubas requested a review from jonathanknowles April 11, 2023 19:15

Jonathan review

175968a

paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch 2 times, most recently from b3c44c7 to 6cde360 Compare April 12, 2023 11:14

paweljakubas requested a review from Anviking April 12, 2023 14:58

Anviking reviewed Apr 12, 2023

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch 4 times, most recently from 990aaff to 2490c5e Compare April 17, 2023 06:46

Anviking reviewed Apr 17, 2023

View reviewed changes

Anviking requested changes Apr 17, 2023

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-2602/shared-wallets-delegation branch from 2490c5e to 76cc989 Compare April 17, 2023 12:44

Base automatically changed from paweljakubas/adp-2602/shared-wallets-delegation to master April 17, 2023 13:53

paweljakubas closed this Apr 20, 2023

paweljakubas deleted the paweljakubas/rewards-withdrawals-shared-wallets branch April 20, 2023 10:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewards in shared wallets #3842

Rewards in shared wallets #3842

paweljakubas commented Apr 7, 2023 •

edited

Loading

jonathanknowles left a comment

jonathanknowles Apr 11, 2023

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023 •

edited

Loading

paweljakubas Apr 11, 2023

jonathanknowles Apr 11, 2023 •

edited

Loading

paweljakubas Apr 11, 2023

Anviking Apr 12, 2023

paweljakubas Apr 12, 2023 •

edited

Loading

Anviking Apr 12, 2023

Anviking Apr 17, 2023

Anviking left a comment

paweljakubas commented Apr 20, 2023

		readWalletCheckpoint ::
		DBLayer IO s k -> WalletId -> ExceptT ErrNoSuchWallet IO (Wallet s)

	_getCachedRewardAccountBalance rewardsObserverFromScriptHash rewardAcct
	_getCachedRewardAccountBalance
	rewardsObserverFromScriptHash rewardAcct

	[ "I cannot construct a delegating transaction for a shared wallet "
	[ "I cannot construct a delegating transaction for a shared wallet"

	(Quantity . fromIntegral . unCoin . W.stakeKeyDeposit $ pp)
	(Coin.toQuantity . W.stakeKeyDeposit $ pp)

Rewards in shared wallets #3842

Rewards in shared wallets #3842

Conversation

paweljakubas commented Apr 7, 2023 • edited Loading

Comments

Issue Number

jonathanknowles left a 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

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

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

jonathanknowles Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking left a comment

Choose a reason for hiding this comment

paweljakubas commented Apr 20, 2023

paweljakubas commented Apr 7, 2023 •

edited

Loading

jonathanknowles Apr 11, 2023 •

edited

Loading

jonathanknowles Apr 11, 2023 •

edited

Loading

paweljakubas Apr 12, 2023 •

edited

Loading