Undelegate with existing rewards #3119

Anviking · 2022-02-10T10:44:54Z

Old workflow:

Automatically withdraw rewards when quitting (both for HW and normal wallets)

New workflow (constructTransaction):

Don't fail with ErrNonNullRewards if "withdrawal": "self" is set when quitting

Comments

cabal v2-test cardano-wallet:integration --test-option=-j --test-option="10" --test-option="--match" --test-option "quit with rewards"

Before fix

    TRANS_NEW_QUIT_02a - Cannot quit with rewards without explicit withdrawal (51966ms)
    TRANS_NEW_QUIT_02b - Can quit with rewards with explicit withdrawal FAILED [1] (51967ms)
  SHELLEY_STAKE_POOLS
    STAKE_POOLS_QUIT_03 - Can quit with rewards FAILED [2] (51966ms)

Failures:

  src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs:2597:24:
  1) API Specifications, NEW_SHELLEY_TRANSACTIONS, TRANS_NEW_QUIT_02b - Can quit with rewards with explicit withdrawal
       uncaught exception: ErrorCall
       getFromResponse failed to get item
       CallStack (from HasCallStack):
         error, called at src/Test/Integration/Framework/DSL.hs:2034:16 in cardano-wallet-core-integration-2022.1.18-inplace:Test.Integration.Framework.DSL
         getFromResponse, called at src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs:2597:24 in cardano-wallet-core-integration-2022.1.18-inplace:Test.Integration.Scenario.API.Shelley.TransactionsNew

  To rerun use: --match "/API Specifications/NEW_SHELLEY_TRANSACTIONS/TRANS_NEW_QUIT_02b - Can quit with rewards with explicit withdrawal/"

  src/Test/Integration/Scenario/API/Shelley/StakePools.hs:537:15:
  2) API Specifications, SHELLEY_STAKE_POOLS, STAKE_POOLS_QUIT_03 - Can quit with rewards
       From the following response: Left
           ( ClientError
               ( Object
                   ( fromList
                       [
                           ( "code"
                           , String "non_null_rewards"
                           )
                       ,
                           ( "message"
                           , String "It seems that you're trying to retire from delegation although you've unspoiled rewards in your rewards account! Make sure to withdraw your 1000000.000000 lovelace first."
                           )
                       ]
                   )
               )
           )
       While verifying value:
         ( Status
             { statusCode = 403
             , statusMessage = "Forbidden"
             }
         , Left
             ( ClientError
                 ( Object
                     ( fromList
                         [
                             ( "code"
                             , String "non_null_rewards"
                             )
                         ,
                             ( "message"
                             , String "It seems that you're trying to retire from delegation although you've unspoiled rewards in your rewards account! Make sure to withdraw your 1000000.000000 lovelace first."
                             )
                         ]
                     )
                 )
             )
         )
       expected: Status {statusCode = 202, statusMessage = "Accepted"}
        but got: Status {statusCode = 403, statusMessage = "Forbidden"}

  To rerun use: --match "/API Specifications/SHELLEY_STAKE_POOLS/STAKE_POOLS_QUIT_03 - Can quit with rewards/"

Randomized with seed 1670117879

Finished in 52.0220 seconds, used 10.2823 seconds of CPU time
3 examples, 2 failures

After fixes:

    TRANS_NEW_QUIT_02a - Cannot quit with rewards without explicit withdrawal (51666ms)
    TRANS_NEW_QUIT_02b - Can quit with rewards with explicit withdrawal (51975ms)
  SHELLEY_STAKE_POOLS
    STAKE_POOLS_QUIT_03 - Can quit with rewards (51975ms)

Issue Number

ADP-1067

paweljakubas · 2022-02-11T12:40:11Z

what about adding :

                , expectField #depositReturned (`shouldBe` Quantity 1000000)
                , expectField #depositTaken (`shouldBe` Quantity 0)

to both StakePools and TransactionNew tests?

Currently fails with: src/Test/Integration/Scenario/API/Shelley/StakePools.hs:537:15: 1) API Specifications, SHELLEY_STAKE_POOLS, STAKE_POOLS_QUIT_xx - Can quit with rewards expected: Status {statusCode = 202, statusMessage = "Accepted"} but got: Status {statusCode = 403, statusMessage = "Forbidden"} from the following response: Left (DecodeFailure "Error in $: parsing Cardano.Wallet.Api.Types.ApiTransaction(ApiTransaction) failed, key \"id\" not found: Response {responseStatus = Status {statusCode = 403, statusMessage = \"Forbidden\"}, responseVersion = HTTP/1.1, responseHeaders = [(\"Transfer-Encoding\",\"chunked\"),(\"Date\",\"Fri, 06 Aug 2021 13:21:52 GMT\"),(\"Server\",\"Warp/3.3.17\"),(\"Content-Type\",\"application/json;charset=utf-8\")], responseBody = \"{\\\"code\\\":\\\"non_null_rewards\\\",\\\"message\\\":\\\"It seems that you're trying to retire from delegation although you've unspoiled rewards in your rewards account! Make sure to withdraw your 1000000000000 lovelace first.\\\"}\", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}") While verifying value: ( Status { statusCode = 403 , statusMessage = "Forbidden" } , Left ( DecodeFailure "Error in $: parsing Cardano.Wallet.Api.Types.ApiTransaction(ApiTransaction) failed, key "id" not found: Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Fri, 06 Aug 2021 13:21:52 GMT"),("Server","Warp/3.3.17"),("Content-Type","application/json;charset=utf-8")], responseBody = "{\"code\":\"non_null_rewards\",\"message\":\"It seems that you're trying to retire from delegation although you've unspoiled rewards in your rewards account! Make sure to withdraw your 1000000000000 lovelace first.\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}" ))

Anviking · 2022-02-11T18:30:51Z

what about adding :

@paweljakubas Done, and I found something interesting:

        quitStakePool @n ctx (w, fixturePassphrase) >>= flip verify
            [ expectResponseCode HTTP.status202
            , expectField #depositTaken (`shouldBe` (Quantity 0))
            , expectField #depositReturned
                (`shouldBe` (Quantity 0))
                    -- FIXME: We would expect 1000000 here;
                    -- seems like a bug!

I pushed a fix to the reclaimIfAny calculation (which didn't seem to care about withdrawals)... let's see if everything is still green.

paweljakubas · 2022-02-11T19:11:22Z

lib/core/src/Cardano/Wallet.hs

+        WithdrawalSelf {} -> return ()
+        _
+            | rewards == Coin 0  -> return ()
+            | otherwise          -> Left $ ErrNonNullRewards rewards


paweljakubas · 2022-02-11T19:11:38Z

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

+        let unsignedTx2 = getFromResponse #transaction rUnsignedTx2
+        verify rUnsignedTx2
+            [ expectField (#coinSelection . #depositsReturned)
+                (`shouldBe` [Quantity 1000000])


paweljakubas · 2022-02-11T19:12:26Z

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

+                ctx (Link.listStakePools arbitraryStake) Empty
+        joinStakePool @n ctx pool (w, fixturePassphrase) >>= flip verify
+            [ expectResponseCode HTTP.status202
+            , expectField #depositTaken (`shouldBe` (Quantity 0))


hmm, shouldn't this be Quantity depositAmt ?

This is one of the fixture rewardWallets which already have registered stake keys. Added a comment to the other test, but can add one here as well.

aha, ok. so this is shelley wallet with gathered some rewards 👍 (---> rewardWallet rather than fixtureWallet). And we do it in order not to wait. Then here we are really rejoining.

ok, I was expecting the flow more like in suite STAKE_POOLS_JOIN_04 - Rewards accumulate

BTW. you need to correct there :

652 -- Can't quite if unspoiled rewards. 653 quitStakePool @n ctx (w, fixturePassphrase) >>= flip verify 654 [ expectResponseCode HTTP.status403 655 , expectErrorMessage errMsg403NonNullReward 656 ]

No anymore valid after your change.

paweljakubas · 2022-02-11T19:13:54Z

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

                  depositValue
              else
                  0
        | otherwise = 0

-    totalInWithoutFee :: Natural


indeed, this was redundant if we take into account withdrawals 👍 And the name was misleading

now thinking about this. we want to detect quitting and receiving deposit back. Shouldn't we have instead totalIn

totalInWithoutWdrl = sum (txOutValue <$> mapMaybe snd (tx ^. #txInputs))

in the end? We are not after detecting direction here

all test passes, so you are right 😅

paweljakubas · 2022-02-11T19:15:00Z

lib/core/test/unit/Cardano/WalletSpec.hs

@@ -459,8 +462,12 @@ prop_guardQuitJoin (NonEmpty knownPoolsList) dlg rewards =
                    knownPools dlg (last knownPoolsList) noRetirementPlanned
                    === Right ()
        Left W.ErrNonNullRewards{} ->
-            label "ErrNonNullRewards"
-                (property $ rewards /= 0)
+            label "ErrNonNullRewards" $


paweljakubas

LGTM, except https://github.com/input-output-hk/cardano-wallet/pull/3119/files#r804961464
It is not forbidden anymore. I was expecting the flow more like in STAKE_POOLS_JOIN_04 - Rewards accumulate but you used rewardWallet . Kinda surprised here. I understand we have wallet with rewards, but still at first I thought when joining I will see depositTaken > 0. Of course, we cannot deregister stake credential without taking rewards home, so when we have rewards to be taken it implies we are staking ...
I checked locally

stack test --ta '-m "TRANS_NEW"' cardano-wallet:integration   OK
stack test --ta '-m "STAKE_POOLS"' cardano-wallet:integration  only STAKE_POOLS_JOIN_04 - Rewards accumulate fails for the obvious reason

Anviking · 2022-02-14T09:06:00Z

bors try

iohk-bors · 2022-02-14T11:03:41Z

try

Build succeeded:

Anviking self-assigned this Feb 10, 2022

Anviking force-pushed the anviking/ADP-1067/undelegateWithExistingRewards branch 3 times, most recently from 4cef1e8 to 9ec842c Compare February 10, 2022 19:36

Anviking marked this pull request as ready for review February 10, 2022 19:45

Anviking added 4 commits February 11, 2022 16:07

Withdraw when quitting in old tx-workflow

f3704a7

Allow quitting with rewards if also withdrawing

70241eb

Add deposit-related expectations

c1aa000

Anviking force-pushed the anviking/ADP-1067/undelegateWithExistingRewards branch from 9ec842c to c1aa000 Compare February 11, 2022 17:54

Fix reclaimIfAny calculation with withdrawals

cb12078

paweljakubas reviewed Feb 11, 2022

View reviewed changes

paweljakubas approved these changes Feb 11, 2022

View reviewed changes

Fix expectation in STAKE_POOLS_JOIN_04

2337136

iohk-bors bot added a commit that referenced this pull request Feb 14, 2022

Try #3119:

220ea6c

Anviking merged commit f56e469 into master Feb 14, 2022

Anviking deleted the anviking/ADP-1067/undelegateWithExistingRewards branch February 14, 2022 11:06

danielmain mentioned this pull request Mar 2, 2022

[DDW-703] Undelegate a wallet input-output-hk/daedalus#2636

Open

36 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undelegate with existing rewards #3119

Undelegate with existing rewards #3119

Anviking commented Feb 10, 2022 •

edited

Loading

paweljakubas commented Feb 11, 2022

Anviking commented Feb 11, 2022

paweljakubas Feb 11, 2022

paweljakubas Feb 11, 2022

paweljakubas Feb 11, 2022

Anviking Feb 11, 2022 •

edited

Loading

paweljakubas Feb 11, 2022

paweljakubas Feb 11, 2022

paweljakubas Feb 11, 2022

paweljakubas Feb 11, 2022 •

edited

Loading

paweljakubas Feb 11, 2022

paweljakubas Feb 11, 2022

paweljakubas left a comment

Anviking commented Feb 14, 2022

iohk-bors bot commented Feb 14, 2022

Undelegate with existing rewards #3119

Undelegate with existing rewards #3119

Conversation

Anviking commented Feb 10, 2022 • edited Loading

Comments

Issue Number

paweljakubas commented Feb 11, 2022

Anviking commented Feb 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Feb 11, 2022 • 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 Feb 11, 2022 • edited Loading

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

Anviking commented Feb 14, 2022

iohk-bors bot commented Feb 14, 2022

try

Anviking commented Feb 10, 2022 •

edited

Loading

Anviking Feb 11, 2022 •

edited

Loading

paweljakubas Feb 11, 2022 •

edited

Loading