Redeem rewards from another wallet #1967

KtorZ · 2020-07-28T02:25:58Z

Issue Number

#1965

Overview

8f97407
📍 let the caller decide how to generate a reward account for signing withdrawals
The goal is to allow a mnemonic to be passed around as a function
argument. If it is, then that account from the mnemonic is used.
Otherwise, it will defaults to the wallet's own account.
e26fc38
📍 make it possible to query rewards of any arbitrary reward account
Yet, the server still query rewards from standard wallet reward
account at the moment. Next commits will make it possible for users to
pass a mnemonic to the API.
657e541
📍 remove duplicated logic between 'readNextWithdrawal' and 'queryRewardBalance'
This partially revert some of the previous commit, but that's for the best.
fc14a3b
📍 extend 'POST' transaction Shelley endpoint to support redeeming rewards
So, now the transaction endpoint accepts two kind of payloads, either a list of payments like before, or simply a source mnemonic from which rewards would be drained / withdrawn.
9161f9f
📍 update 'postTransaction' handler to handle payments and withdrawals
7a71a7e
📍 separate 'toChimericAccount' from 'HasRewardAccount' type-class
This makes it slightly easier to deal with all these constraints in the API, in particular in the context of withdrawals.
Also, I've completely removed the 'HasRewardAccount' constraint from the 'readChimericAccount' function. Instead, that function will
dynamically check that the type is a Shelley type and fails with a proper error otherwise.
04c96ea
📍 actually get rid of 'HasRewardAccount' type-class altogether
This type class only existed in order to be able to get a reward account from a sequential state to assess whether an account was ours or not.
We had to defined an unsound 'HasRewardAccount' on 'IcarusKey' because of that and carry this instance in many places in the API. Instead, we
can simply carry a 'IsOurs _ ChimericAccount' constraints up to API instantiation.
03da476
📍 unify withdrawal requests frmo self or external wallets under one common API
This is so-to-speak a breaking change compared to what we just released yet, it's for the best. Now there's one consistent way of creating withdrawals! This also gets rid of a few edge-case that are not possible anymore by design.
3e0e9db
📍 add pre-funded wallets with rewards using MIR certificates to the integration cluster
3183a7f
📍 add integration scenarios for reward redemption
Trying various scenarios, from self and from other wallets.
d3ebc72
📍 prevent users from withdrawing from empty reward accounts.

Comments

paweljakubas · 2020-07-28T07:57:38Z

lib/core/src/Cardano/Wallet.hs

-        <*> fmap getQuantity (readDelegationRewardBalance pk)
+    -> Either WalletId SomeMnemonic
+    -> ExceptT ErrFetchRewards IO (Quantity "lovelace" Word64)
+readNextWithdrawal ctx wid src = db & \DBLayer{..} -> do


src was unexpected name for me. tbh. maybe acctSrc, chimericOrigin ?

Good point. My creativity for names at 4 a.m. was somewhat diminished 😂

paweljakubas · 2020-07-28T07:59:10Z

lib/core/src/Cardano/Wallet.hs

+        Left (ErrGetAccountBalanceNetworkUnreachable e) ->
+            Left $ ErrFetchRewardsNetworkUnreachable e
+
+    fromOwnWallet


paweljakubas · 2020-07-28T07:59:20Z

lib/core/src/Cardano/Wallet.hs

+    fromOwnWallet
+        :: WalletId
+        -> ExceptT ErrFetchRewards IO ChimericAccount
+    fromOwnWallet _wid = db & \DBLayer{..} -> do


paweljakubas · 2020-07-28T08:00:45Z

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

@@ -159,6 +165,25 @@ instance Functor m => Functor (NetworkLayer m target) where
        { nextBlocks = fmap (fmap f) . nextBlocks nl
        }

+-- Fetching the account balance may sometimes fail, especially on epoch
+-- boundaries. Therefore, we add some retrying mecanism here to cope with


Suggested change

-- boundaries. Therefore, we add some retrying mecanism here to cope with

-- boundaries. Therefore, we add some retrying mechanism here to cope with

paweljakubas · 2020-07-28T08:05:39Z

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

+    -> ChimericAccount
+    -> ExceptT ErrGetAccountBalance IO (Quantity "lovelace" Word64)
+getAccountBalanceRetry nl account = ExceptT $
+    retrying policy decision (const $ runExceptT $ getAccountBalance nl account)


I assume here, that there will be a moment, where cardano-node will be able to discover itn rewards basing on chimeric account? so itn and shelley chimeric account will be married, the same format, etc ? and we can reuse the same local query infrastructure already present?

Indeed. In principle, on the 3rd of August, rewards will be transferred to the Mainnet based on the same public key that was used on the ITN.

paweljakubas · 2020-07-28T08:12:26Z

so far so good (although I liked more fromOwnWallet and fromExternalWallet and they changed in last commit ;-)
of course, waiting for proof of concept in integration testing (the question is whether itn has been absorbed already in cardano-node and it is possible now )

paweljakubas · 2020-07-28T08:29:28Z

lib/core/src/Cardano/Wallet/Api/Types.hs

@@ -491,10 +499,27 @@ data PostTransactionData (n :: NetworkDiscriminant) = PostTransactionData
    , passphrase :: !(ApiT (Passphrase "lenient"))
    } deriving (Eq, Generic, Show)

+data PostWithdrawalData = PostWithdrawalData
+    { source :: !(ApiMnemonicT '[15,18,21,24])


why not to use mnemonicSentence like in other places?

paweljakubas · 2020-07-28T08:29:43Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+    } deriving (Eq, Generic, Show)
+
+newtype PostWithdrawalFeeData = PostWithdrawalFeeData
+    { source :: ApiMnemonicT '[15,18,21,24]


here also, why not to use mnemonicSentence like in other places?

paweljakubas · 2020-07-28T08:30:12Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+instance DecodeAddress t => FromJSON (PostPaymentOrWithdrawalData t) where
+    parseJSON val = flip (withObject "PostPaymentOrWithdrawalData") val $ \o -> do
+        case (HM.lookup "source" o, HM.lookup "payments" o) of
+            (Just{}, Just{})   -> empty


paweljakubas · 2020-07-28T08:31:14Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+    toJSON (PostPaymentOrWithdrawalFeeData (Left x))  = toJSON x
+    toJSON (PostPaymentOrWithdrawalFeeData (Right x)) = toJSON x
+instance DecodeAddress t => FromJSON (PostPaymentOrWithdrawalFeeData t) where
+    parseJSON val = flip (withObject "PostPaymentOrWithdrawalFeeData") val $ \o -> do


what about reusing what was done in impl of instance DecodeAddress t => FromJSON (PostPaymentOrWithdrawalData t) here?

Because the JSON object isn't exactly the same (FeeData has no passphrase).

paweljakubas · 2020-07-28T08:32:03Z

lib/core/src/Cardano/Wallet/Primitive/Model.hs

        => ([(Tx, TxMeta)], UTxO)
        -> Tx
        -> State s ([(Tx, TxMeta)], UTxO)
    applyTx (!txs, !u) tx = do
        ourU <- state $ utxoOurs tx
        let ourIns = Set.fromList (inputs tx) `Set.intersection` dom (u <> ourU)
        let u' = (u <> ourU) `excluding` ourIns
-        ourWithdrawals <- mapMaybeM ourChimericAccount $ Map.toList $ withdrawals tx
+        let wdrls = fromIntegral . getCoin <$> Map.elems (withdrawals tx)


could you explain this change?

Yes. Since we can now have withdrawals in our wallet that aren't from our chimeric account (but will still contribute positively to the input balance of a transaction), we need to account for pretty much all withdrawals in transactions (or otherwise, more complicated, "remember" all chimeric account we've made withdrawals from...).

This will work fine until we introduce multisig (where some inputs will not be ours... right now, if one input is ours, then they all are so the problem is quite simplified). By then, we'll have to review this.

paweljakubas · 2020-07-28T09:12:43Z

specifications/api/swagger.yaml

+    ApiPostWithdrawalData: &ApiPostWithdrawalData
+      type: object
+      required:
+        - source


we use - mnemonic_sentence all around for this bit

…thdrawals The goal is to allow a mnemonic to be passed around as a function argument. If it is, then that account from the mnemonic is used. Otherwise, it will defaults to the wallet's own account.

Yet, the server still query rewards from standard wallet reward account at the moment. Next commits will make it possible for users to pass a mnemonic to the API.

…Balance' This partially revert some of the previous commit, but that's for the best.

So, now the transaction endpoint accepts two kind of payloads, either a list of payments like before, or simply a source mnemonic from which rewards would be drained / withdrawn.

This makes it slightly easier to deal with all these constraints in the API, in particular in the context of withdrawals. Also, I've completely removed the 'HasRewardAccount' constraint from the 'readChimericAccount' function. Instead, that function will dynamically check that the type is a Shelley type and fails with a proper error otherwise.

This type class only existed in order to be able to get a reward account from a sequential state to assess whether an account was ours or not. We had to defined an unsound 'HasRewardAccount' on 'IcarusKey' because of that and carry this instance in many places in the API. Instead, we can simply carry a 'IsOurs _ ChimericAccount' constraints up to API instantiation.

…mon API This is so-to-speak a breaking change compared to what we just released yet, it's for the best. Now there's one consistent way of creating withdrawals! This also gets rid of a few edge-case that are not possible anymore by design.

…egration cluster

Trying various scenarios, from self and from other wallets.

KtorZ · 2020-07-30T22:39:56Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

@@ -1240,6 +1254,148 @@ spec = do
            [ expectResponseCode @IO HTTP.status404
            , expectErrorMessage (errMsg404NoWallet wid)
            ]
+
+    it "SHELLEY_TX_REDEEM_01 - Can redeem rewards from self" $ \ctx -> do
+        (wSrc,_) <- rewardWallet ctx


cc @piotr-iohk

KtorZ · 2020-07-30T22:48:22Z

bors try

iohk-bors · 2020-07-30T23:43:04Z

try

Build succeeded

paweljakubas · 2020-07-31T07:48:46Z

lib/byron/test/integration/Cardano/Wallet/Byron/Faucet.hs


 initFaucet :: IO Faucet
 initFaucet = Faucet
    <$> newMVar seqMnemonics
    <*> newMVar icaMnemonics
    <*> newMVar rndMnemonics
+    <*> newMVar mirMnemonics


I generated 10 of them, used 2 or 3 I think in the scenarios I wrote, so there is still some room for some :p

paweljakubas · 2020-07-31T07:50:55Z

lib/core-integration/src/Test/Integration/Framework/DSL.hs

@@ -785,6 +786,27 @@ emptyWalletWith ctx (name, passphrase, addrPoolGap) = do
    expectResponseCode @IO HTTP.status201 r
    return (getFromResponse id r)

+rewardWallet :: Context t -> IO (ApiWallet, Mnemonic 24)


why not to call it readMIRwallet to better reflect its nature?

I was shared between calling everything MIR (which is sort of cardano-node specific terminology) or simply Reward (which is more generic).

I ended up referring to "MIR" in the setup code, because it makes it clearer to what it connects behind the scene, but resorted to "reward" for everything in the DSL and scenarios. That the rewards come from MIR certificates is sort of an implementation detail.

paweljakubas · 2020-07-31T07:52:39Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

+                "passphrase": #{fixturePassphrase}
+            }|]
+
+        -- Withdraw rewards from the other wallet.


paweljakubas · 2020-07-31T07:52:54Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

+                    (`shouldBe` Quantity 0)
+                ]
+
+        -- Try withdrawing AGAIN, rewards that aren't there anymore.


paweljakubas · 2020-07-31T07:53:24Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

+            , expectErrorMessage errMsg403WithdrawalNotWorth
+            ]
+
+    it "SHELLEY_TX_REDEEM_05 - Can't redeem rewards using byron wallet" $ \ctx -> do


paweljakubas · 2020-07-31T07:54:49Z

lib/core/src/Cardano/Wallet.hs

                    minFee policy (mempty { withdrawal })
                    -
                    minFee policy mempty

-            pure $ if toInteger withdrawal < 2 * costOfWithdrawal
+            in
+                if toInteger withdrawal < 2 * costOfWithdrawal


could you recall me why we have here 2 * costOfWithdrawal , ie., why 2 ?

The rationale was that: "withdrawal should bring at least much value as they cost.".

This is to prevent withdrawals from either loosing money, or being consumed too early when they're really small (imagine an account with 0.31 Ada, and the withdrawal cost 0.3 Ada. If we were just ensuring the withdrawal to be > costOfWithdrawal (without 2 *), then the resulting withdrawal would be 0.01 Ada :/ .. And it really feels not worth it given the fees. Hence the times two.

paweljakubas · 2020-07-31T07:58:25Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+        ErrWithdrawalNotWorth ->
+            apiError err403 WithdrawalNotWorth $ mconcat
+                [ "I've noticed that you're requesting a withdrawal from an "
+                , "account that is either empty or doesn't have a balance big "


it handles also situations, where we got already rewards... saying that account is empty covers this. We don't want to say explicitly something like : presumably the account was already withdrawn ?

Well, an account could be empty for two reasons: either it hasn't received rewards yet, or it has already withdrawn them. I didn't feel like emphasizing on any of these aspects but we could indeed.

paweljakubas · 2020-07-31T07:59:24Z

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation.hs

@@ -315,6 +318,12 @@ class HardDerivation key => SoftDerivation (key :: Depth -> * -> *) where
        -> Index 'Soft 'AddressK
        -> key 'AddressK XPub

+-- | Derivation of a reward account, as a type-class because different between
+-- key types (in particular, Jörmungandr vs Shelley).
+class ToChimericAccount k where


this should also be reflected in cardano-addresses ?

cardano-addresses doesn't actually treat with chimeric account or stake address at all at the moment. We should eventually though.

yes, I was saying about that. We need to remember this in the context of cardano-addresses

paweljakubas · 2020-07-31T08:01:05Z

lib/shelley/test/data/cardano-node-shelley/shelley-genesis.yaml

@@ -40,7 +40,7 @@ genDelegs:
  8ae01cab15f6235958b1147e979987bbdb90788f7c4e185f1632427a:
    delegate: b7bf59bb963aa785afe220f5b0d3deb826fd0bcaeeee58cb81ab443d
    vrf: 4ebcf8b4c13c24d89144d72f544d1c425b4a3aa1ace30af4eb72752e75b40d3e
-updateQuorum: 5
+updateQuorum: 1


could you explain why this change was needed?

Ah! Took me some time to figure that out (still dreaming about a documentation of the genesis parameters one day 🙏).
MIR certificates are used to transfer rewards to pre-registered stake key. This is not an operation that anyone can do (that would be sort of problematic 😅), but only the core delegates can (same goes for other "governance" operations like updating the protocol parameters).

This updateQuorum parameters controls how many genesis delegate signatures are required for a governance type of request to be considered valid on-chain. Since we only have a single BFT leader in this setup, well, we can only have at most one signature! Thus I had to tweak this so that our BFT leader could act in full dictatorial mode :)

paweljakubas · 2020-07-31T08:02:52Z

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

+    arbitrary = PostTransactionData
+        <$> arbitrary
+        <*> arbitrary
+        <*> elements [Just SelfWithdrawal, Nothing]


we don't want to include external withdrawals here?

This is a bit tricky because the swagger2 Haskell libraries doesn't support the oneOf constructor :( ... And here, the swagger schema is a oneOf between a string and a mnemonic sentence; so I had to pick one to not overly complexifies things here; since the mnemonic sentences schema and JSON are already well-tested through other API types, I opted for the self withdrawal one which is completely new and only exists in this context.

paweljakubas

very persuasive test scenarios! I like refactoring of ApiPostTransactionData and how posting tx api changed, ie that withdrawal was embedded in PostTransactionData***. we have breaking change, but I believe it is worth it

KtorZ requested a review from a team July 28, 2020 02:25

KtorZ self-assigned this Jul 28, 2020

paweljakubas reviewed Jul 28, 2020

View reviewed changes

KtorZ force-pushed the KtorZ/1965/itn-withdrawals branch from 30f2d38 to 7c8289c Compare July 29, 2020 21:57

KtorZ added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jul 29, 2020

KtorZ marked this pull request as ready for review July 29, 2020 22:06

KtorZ added 11 commits July 31, 2020 00:38

let the caller decide how to generate a reward account for signing wi…

8f97407

…thdrawals The goal is to allow a mnemonic to be passed around as a function argument. If it is, then that account from the mnemonic is used. Otherwise, it will defaults to the wallet's own account.

make it possible to query rewards of any arbitrary reward account

e26fc38

Yet, the server still query rewards from standard wallet reward account at the moment. Next commits will make it possible for users to pass a mnemonic to the API.

remove duplicated logic between 'readNextWithdrawal' and 'queryReward…

657e541

…Balance' This partially revert some of the previous commit, but that's for the best.

extend 'POST' transaction Shelley endpoint to support redeeming rewards

fc14a3b

So, now the transaction endpoint accepts two kind of payloads, either a list of payments like before, or simply a source mnemonic from which rewards would be drained / withdrawn.

update 'postTransaction' handler to handle payments and withdrawals

9161f9f

add pre-funded wallets with rewards using MIR certificates to the int…

3e0e9db

…egration cluster

add integration scenarios for reward redemption

3183a7f

Trying various scenarios, from self and from other wallets.

prevent users from withdrawing from empty reward accounts.

d3ebc72

KtorZ force-pushed the KtorZ/1965/itn-withdrawals branch from 7c8289c to d3ebc72 Compare July 30, 2020 22:39

KtorZ commented Jul 30, 2020

View reviewed changes

iohk-bors bot added a commit that referenced this pull request Jul 30, 2020

Try #1967:

6668ffc

paweljakubas reviewed Jul 31, 2020

View reviewed changes

paweljakubas approved these changes Jul 31, 2020

View reviewed changes

KtorZ merged commit 5af6e35 into master Jul 31, 2020

KtorZ deleted the KtorZ/1965/itn-withdrawals branch July 31, 2020 08:10

KtorZ mentioned this pull request Jul 31, 2020

Redeem ITN rewards from mainet wallet #1965

Closed

	-- boundaries. Therefore, we add some retrying mecanism here to cope with
	-- boundaries. Therefore, we add some retrying mechanism here to cope with

Redeem rewards from another wallet #1967

Redeem rewards from another wallet #1967

Conversation

KtorZ commented Jul 28, 2020 • edited Loading

Issue Number

Overview

Comments

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

paweljakubas commented Jul 28, 2020

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

paweljakubas Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Jul 30, 2020

iohk-bors bot commented Jul 30, 2020

try

Build succeeded

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

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

KtorZ commented Jul 28, 2020 •

edited

Loading

paweljakubas Jul 28, 2020 •

edited

Loading