Rewards detection in multisig #3872

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

Extending shared state
add readRewardAccount
add manageSharedBalance
use manageSharedBalance
impl IsOurs for RewardAccount
extending integration testing (joining/rejoining)
redefine RewardAccount and deal with code changes throughout all codebase

Comments

Issue Number

Anviking

LGTM, but just realised concern with roundtrip of RewardAccount, and there are some fromJust which shouldn't be needed

Anviking · 2023-04-21T17:34:34Z

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

+data RewardAccountSource
+    = RewardAccountFromKeyHash
+    | RewardAccountFromScriptHash
+    deriving (Eq, Show)
+


Shouldn't be needed any longer

Suggested change

data RewardAccountSource

= RewardAccountFromKeyHash

| RewardAccountFromScriptHash

deriving (Eq, Show)

Anviking · 2023-04-21T17:37:09Z

lib/wallet/test/unit/Cardano/Wallet/Primitive/Delegation/StateSpec.hs

+    show (CmdAdversarialReg (FromKeyHash a)) = "CmdAdversarialReg " <> B8.unpack a
+    show (CmdAdversarialReg (FromScriptHash a)) = "CmdAdversarialReg " <> B8.unpack a


These changes look inconvenient, but still preferable to having two reward observers I think.

Anviking · 2023-04-21T17:43:21Z

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

+                , "detection. Either there is db malfunction or managing rewards "
+                , "was used for shared wallets missing delegation template."


I'm guessing this error should never hit users of the API (if manageSharedRewardBalance is only called for wallets with delegation templates). Should it be err500 instead then?

indeed, err500 is better here

Anviking · 2023-04-21T17:44:49Z

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

+            verify r
+                [ expectField
+                      (#balance . #reward)
+                      (.> (Quantity 0))


Anviking · 2023-04-21T17:50:35Z

lib/wallet/src/Cardano/Wallet.hs

+manageSharedRewardBalance
+    :: forall n block
+     . Tracer IO WalletWorkerLog
+    -> NetworkLayer IO block
+    -> DBLayer IO (SharedState n SharedKey) SharedKey
+    -> WalletId
+    -> IO ()
+manageSharedRewardBalance tr' netLayer db@DBLayer{..} wid = do


Could parts common to both manageSharedRewardBalance and manageRewardBalance be factored out?

Anviking · 2023-04-21T17:55:04Z

lib/wallet/src/Cardano/Wallet.hs

+            when (isNothing acctM) $
+                throwE ErrFetchRewardsMissingRewardAccount
+            liftIO $ getCachedRewardAccountBalance netLayer (fst $ fromJust acctM)


No need for fromJust:

Suggested change

when (isNothing acctM) $

throwE ErrFetchRewardsMissingRewardAccount

liftIO $ getCachedRewardAccountBalance netLayer (fst $ fromJust acctM)

case acctM of

Nothing -> throwE ErrFetchRewardsMissingRewardAccount

Just acct -> liftIO $ getCachedRewardAccountBalance netLayer (fst acct)

Anviking · 2023-04-24T11:14:07Z

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

+                            liftHandler $ throwE ErrConstructTxDelegationInvalid
+                        let path = snd $ fromJust res


Same as other comment — fromJust shouldn't be needed.

Anviking · 2023-04-24T11:21:25Z

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

+                delCerts <- case optionalDelegationAction of
+                    Nothing -> pure Nothing
+                    Just action -> do
+                        res <- liftHandler $ W.readSharedRewardAccount @n db wid


At this point in the flow, we've already managed to create a balanced transaction, so I believe the ErrConstructTxDelegationInvalid would never been thrown from here. (not sure if there's an easy fix though)

you are right. ErrConstructTxDelegationInvalid will never be thrown here. If ever, it would be caught earlier. removed that.

Anviking · 2023-04-24T11:26:38Z

specifications/api/swagger.yaml

+  properties:
+    message:
+      type: string
+      description: May occur when there is a missing reward account but wallet participates in staking.


Same as in the other comment, users would run into ErrDelegationInvalid but never this, modulo DB malfunctions?

yes, MissingRewardAccount is now err500 and can occur when there is no reward account in db (for example db malfunction), and DelegationInvalid is user error (err403) occurring when user instantiate shared wallet with only spending script and creates tx with delegation action.

Anviking · 2023-04-24T11:54:56Z

lib/wallet/src/Cardano/Wallet/Primitive/Types/RewardAccount.hs


 instance FromText RewardAccount where
-    fromText = fmap (RewardAccount . getHash @"RewardAccount") . fromText
+    fromText = fmap (FromKeyHash . getHash @"RewardAccount") . fromText


🤔 This wouldn't roundtrip... Could this be problematic for the DB?

If this is indeed a problem, maybe there's a middle ground between having RewardAccount = FromKeyHash ByteString | FromScriptHash ByteString everywhere and having two newRewardBalanceFetcher — e.g. having the same RewardAccount as before but letting newRewardBalanceFetcher operate on Either RewardAccount RewardAccount (or something isomorphic) as opposed to having two of them?

after some thought and having in mind db migrations I come up with the following solution:

instance ToText RewardAccount where toText (FromKeyHash bs) = T.cons 'k' . toText . Hash @"RewardAccount" $ bs toText (FromScriptHash bs) = T.cons 's' . toText . Hash @"RewardAccount" $ bs instance FromText RewardAccount where fromText txt = case T.splitAt 1 txt of ("s", txt') -> fmap (FromScriptHash . getHash @"RewardAccount") . fromText $ txt' ("k", txt') -> fmap (FromKeyHash . getHash @"RewardAccount") . fromText $ txt' _ -> -- for backward compatibility when there is already db fmap (FromKeyHash . getHash @"RewardAccount") . fromText $ txt

hash is represented as hex so 0-9af alphabet. for key we prepend k for script s so outside the alphabet. Also to be backward compatible we fromText to key version (as this is one we should care atm). core unit tests passes. Old data type with all consequences stays. I think we should be good.

Anviking · 2023-04-26T13:26:54Z

lib/wallet/src/Cardano/Wallet/Primitive/Types/RewardAccount.hs

+        _ -> -- for backward compatibility when there is already db
+            fmap (FromKeyHash . getHash @"RewardAccount") . fromText $ txt


Btw: is there a roundtrip test for this instance? One that would have caught the previous failure to roundtrip?

Anviking

lgtm

Anviking · 2023-04-27T09:28:01Z

lib/wallet/src/Cardano/Wallet/Primitive/Types/RewardAccount.hs

+        _ -> -- for backward compatibility when there is already db
+            fmap (FromKeyHash . getHash @"RewardAccount") . fromText $ txt


Btw: is there a roundtrip test for this instance? One that would have caught the previous failure to roundtrip?

paweljakubas · 2023-04-27T10:40:08Z

yes, there is roundtrip for "ApiRewardAccount" which is newtype wrapper for "RewardAccount". Still, I extended Arbitrary RewardAccount which now uses both data constructors.

paweljakubas · 2023-04-27T10:40:17Z

bors r+

3872: Rewards detection in multisig r=paweljakubas a=paweljakubas  - [x] Extending shared state - [x] add readRewardAccount - [x] add manageSharedBalance - [x] use manageSharedBalance - [x] impl IsOurs for RewardAccount - [x] extending integration testing (joining/rejoining) - [x] redefine RewardAccount and deal with code changes throughout all codebase ### Comments  ### Issue Number adp-2603  Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>

iohk-bors · 2023-04-27T11:51:48Z

Build failed:

buildkite/cardano-wallet

piotr-iohk · 2023-04-27T13:02:50Z

@paweljakubas I see some integration tests fail with error 500 from the API:

That's embarrassing. Your wallet looks good, but I couldn't open a new database to store its data. This is unexpected and likely not your fault. Perhaps, check your filesystem's permissions or available space?

I observe the same when trying to create "incomplete" shared wallet on preprod (fails only when delegation template is incomplete, still seems to work fine for incomplete payment template):

curl -X POST http://localhost:8090/v2/shared-wallets \
-d '{
   "mnemonic_sentence":[
      "camera",
      "they",
       ....
      "kit"
   ],
   "passphrase":"Secure Passphrase",
   "name":"My Test Shared Wallet",
   "account_index":"0H",
   "payment_script_template":{
      "cosigners":{
         "cosigner#0":"self"
      },
      "template":{
         "all":[
            "cosigner#0"
         ]
      }
   },
   "delegation_script_template":{
      "cosigners":{
         "cosigner#0":"self"
      },
      "template":{
         "all":[
            "cosigner#0",
            "cosigner#1" <----------- delegation template "incomplete" as it has one extra cosigner that can be patched later on
         ]
      }
   }
}' \
-H "Content-Type: application/json"

👇

{
  "code": "unexpected_error",
  "message": "That's embarrassing. Your wallet looks good, but I couldn't open a new database to store its data. 
            This is unexpected and likely not your fault. Perhaps, check your filesystem's permissions or available space?"
}

In the log there is:

CallStack (from HasCallStack):
  error, called at src/Cardano/Wallet/Address/Derivation/SharedKey.hs:144:24 in cardano-wallet-2023.4.14-2N7szGvXGvP3BR5Wn8oQmO:Cardano.Wallet.Address.Derivation.SharedKey
  toKeyHash, called at src/Cardano/Wallet/Address/Derivation/SharedKey.hs:130:54 in cardano-wallet-2023.4.14-2N7szGvXGvP3BR5Wn8oQmO:Cardano.Wallet.Address.Derivation.SharedKey
[cardano-wallet.wallet-engine:Error:160] [2023-04-27 12:58:03.24 UTC] Worker has exited: Unhandled exception: Impossible: cosigner without accXPpub.
CallStack (from HasCallStack):
  error, called at src/Cardano/Wallet/Address/Derivation/SharedKey.hs:144:24 in cardano-wallet-2023.4.14-2N7szGvXGvP3BR5Wn8oQmO:Cardano.Wallet.Address.Derivation.SharedKey
  toKeyHash, called at src/Cardano/Wallet/Address/Derivation/SharedKey.hs:130:54 in cardano-wallet-2023.4.14-2N7szGvXGvP3BR5Wn8oQmO:Cardano.Wallet.Address.Derivation.SharedKey
[cardano-wallet.api-server:Error:158] [2023-04-27 12:58:03.24 UTC] [RequestId 19] POST /v2/shared-wallets 500 Internal Server Error in 0.19656104s

paweljakubas · 2023-04-28T12:39:37Z

bors r+

3872: Rewards detection in multisig r=paweljakubas a=paweljakubas  - [x] Extending shared state - [x] add readRewardAccount - [x] add manageSharedBalance - [x] use manageSharedBalance - [x] impl IsOurs for RewardAccount - [x] extending integration testing (joining/rejoining) - [x] redefine RewardAccount and deal with code changes throughout all codebase ### Comments  ### Issue Number adp-2603  Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>

iohk-bors · 2023-04-28T12:51:20Z

Build failed:

buildkite/cardano-wallet

clean

paweljakubas · 2023-04-28T12:54:54Z

bors r+

iohk-bors · 2023-04-28T13:29:47Z

Build succeeded:

buildkite/cardano-wallet

…a=paweljakubas  - [x] Extending shared state - [x] add readRewardAccount - [x] add manageSharedBalance - [x] use manageSharedBalance - [x] impl IsOurs for RewardAccount - [x] extending integration testing (joining/rejoining) - [x] redefine RewardAccount and deal with code changes throughout all codebase ### Comments  ### Issue Number adp-2603  Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io> Source commit: 56b346a

paweljakubas requested a review from Anviking April 20, 2023 10:08

paweljakubas self-assigned this Apr 20, 2023

paweljakubas mentioned this pull request Apr 20, 2023

Rewards in shared wallets #3842

Closed

6 tasks

paweljakubas force-pushed the paweljakubas/adp-2603/rewards-multisig branch 3 times, most recently from cd9682c to 8385923 Compare April 24, 2023 07:45

Anviking reviewed Apr 24, 2023

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-2603/rewards-multisig branch from c2cc8f6 to 59e1d32 Compare April 26, 2023 13:23

Anviking reviewed Apr 26, 2023

View reviewed changes

paweljakubas requested a review from Anviking April 26, 2023 13:28

paweljakubas force-pushed the paweljakubas/adp-2603/rewards-multisig branch from 59e1d32 to 294e8c8 Compare April 26, 2023 14:54

Anviking approved these changes Apr 27, 2023

View reviewed changes

paweljakubas force-pushed the paweljakubas/adp-2603/rewards-multisig branch from 294e8c8 to 10a5d8b Compare April 27, 2023 10:37

paweljakubas force-pushed the paweljakubas/adp-2603/rewards-multisig branch from 10a5d8b to 10fb99a Compare April 28, 2023 12:39

paweljakubas added 8 commits April 28, 2023 14:53

add integration detecting rewards

cc09b7a

add rewardAcct to SharedState

204e2c0

add readSharedRewardAccount

e8ab4fd

clean

add manageSharedRewardBalance

9f496f9

use manageSharedBalance in Server

97c1d18

introduce toScriptHashStakeCredential

54c78ba

clean

enhancements

8f03f8c

clean

isOurs for RewardAccount

52341f9

paweljakubas added 12 commits April 28, 2023 14:53

expand on integration test - rejoin

6cde30a

add rest expectations for rejoin

97e565e

clean

post rebase fix

6d992c7

redefine RewardAccount

e49f629

hlint

0e2d745

add MissingRewardAccount in swagger

17186f3

post rebase fixes

f374a84

review fixes - part 1

3dfbff4

fix ToText and FromText of RewardAccount

64c6594

improve Arbitrary RewardAccount

2978234

be even less intrusive for shelley style

d81d77f

fix how shared state is activated

352dda7

paweljakubas force-pushed the paweljakubas/adp-2603/rewards-multisig branch from 10fb99a to 352dda7 Compare April 28, 2023 12:54

iohk-bors bot merged commit 56b346a into master Apr 28, 2023

iohk-bors bot deleted the paweljakubas/adp-2603/rewards-multisig branch April 28, 2023 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewards detection in multisig #3872

Rewards detection in multisig #3872

paweljakubas commented Apr 20, 2023 •

edited

Loading

Anviking left a comment

Anviking Apr 21, 2023

paweljakubas Apr 26, 2023

Anviking Apr 21, 2023

Anviking Apr 21, 2023

paweljakubas Apr 26, 2023

Anviking Apr 21, 2023

Anviking Apr 21, 2023

paweljakubas Apr 26, 2023

Anviking Apr 21, 2023

Anviking Apr 24, 2023

paweljakubas Apr 26, 2023

Anviking Apr 24, 2023 •

edited

Loading

paweljakubas Apr 26, 2023

Anviking Apr 24, 2023

paweljakubas Apr 26, 2023

Anviking Apr 24, 2023

paweljakubas Apr 26, 2023

Anviking Apr 26, 2023

Anviking Apr 27, 2023 •

edited

Loading

Anviking left a comment

Anviking Apr 27, 2023 •

edited

Loading

paweljakubas commented Apr 27, 2023

paweljakubas commented Apr 27, 2023

iohk-bors bot commented Apr 27, 2023

piotr-iohk commented Apr 27, 2023

paweljakubas commented Apr 28, 2023

iohk-bors bot commented Apr 28, 2023

paweljakubas commented Apr 28, 2023

iohk-bors bot commented Apr 28, 2023

		show (CmdAdversarialReg (FromKeyHash a)) = "CmdAdversarialReg " <> B8.unpack a
		show (CmdAdversarialReg (FromScriptHash a)) = "CmdAdversarialReg " <> B8.unpack a

		, "detection. Either there is db malfunction or managing rewards "
		, "was used for shared wallets missing delegation template."

		liftHandler $ throwE ErrConstructTxDelegationInvalid
		let path = snd $ fromJust res

		_ -> -- for backward compatibility when there is already db
		fmap (FromKeyHash . getHash @"RewardAccount") . fromText $ txt

Rewards detection in multisig #3872

Rewards detection in multisig #3872

Conversation

paweljakubas commented Apr 20, 2023 • edited Loading

Comments

Issue Number

Anviking 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

Anviking Apr 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Anviking left a comment

Choose a reason for hiding this comment

Anviking Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

paweljakubas commented Apr 27, 2023

paweljakubas commented Apr 27, 2023

iohk-bors bot commented Apr 27, 2023

piotr-iohk commented Apr 27, 2023

paweljakubas commented Apr 28, 2023

iohk-bors bot commented Apr 28, 2023

paweljakubas commented Apr 28, 2023

iohk-bors bot commented Apr 28, 2023

paweljakubas commented Apr 20, 2023 •

edited

Loading

Anviking Apr 24, 2023 •

edited

Loading

Anviking Apr 27, 2023 •

edited

Loading

Anviking Apr 27, 2023 •

edited

Loading