Allow redelegation to the same pool in Conway #4562

paweljakubas · 2024-04-23T12:08:12Z

Comments

In order to run integration tests do the following:
(a) Babbage

just integration-tests-cabal-match TRANS_NEW_JOIN_01f

(b) Conway

just conway-integration-tests-cabal-match TRANS_NEW_JOIN_01f

Issue Number

adp 3312

HeinrichApfelmus · 2024-04-25T13:02:42Z

lib/api/src/Cardano/Wallet/Api/Http/Shelley/Server.hs

Ah, I think that this file should not be modified. 😅 The reason is that allowing or disallowing delegation is the main concern of the function guardJoin — any changes should be localized to that function or its immediate context. (The next larger context for guardJoin is the function joinStakePoolDelegationAction).

That said, we have split the stake pool delegation and the voting parts a little bit — and they are joined only here, which is not a good separation of concerns. Ideally, the logic would be joined at voteAction and handleDelegationRequest. I see two options to choose from:

Make a function handleVoteAndDelegateRequest in the module Cardano.Wallet.IO.Delegation which merges the handleDelegationRequest and voteAction, and returns IO (Tx.DelegationAction, Tx.VotingAction). This function handles the interaction between the two concepts. This moves the concern out of the Server module where it doesn't belong.

Go back to the previous version where we unconditionally allow joining a separate stake pool. This would simplify the implementation tremendously (at the expense of having a future change do 1 anyway.)

I leave the choice up to you. 🤓

(PS: Once the concern has been moved to handleVoteAndDelegateRequest, I will ask you to split off a pure function again, along the lines of how joinStakePoolDelegationAction has been split into a pure variant and an IO variant.)

added handleVoteAndDelegateRequest 👍

HeinrichApfelmus · 2024-04-25T13:07:04Z

lib/integration/scenarios/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                }]
+            }|]
+        rTx2 <- request @(ApiConstructTransaction n) ctx
+            (Link.createUnsignedTransaction @'Shelley src) Default delegationJoin


Suggested change

(Link.createUnsignedTransaction @'Shelley src) Default delegationJoin

Alas, Daedalus still uses the old joinStakePool endpoint

https://cardano-foundation.github.io/cardano-wallet/api/edge/#operation/joinStakePool

Can you write the additional integration tests to be only about this endpoint?

I don't think it makes sense to test Link.createUnsignedTransaction at this level — it's a kitchen-sink function, other tests are checking that it's behavior hasn't changed too much; I think that is sufficient.

added joinStakePool tests showing the case for both Babbage and Conway era. I would argue testing in new tx workflow is also crucial as we want to be consistent here and outlaw some action (those that are redundant and incur fees)

I would argue testing in new tx workflow is also crucial as we want to be consistent here and outlaw some action (those that are redundant and incur fees)

Well in this case, I would argue that a user of createUnsignedTransaction is a programmer and knows what they are doing, whereas joinStakePool is a proxy for actions from Daedalus users. So the latter would benefit from not incurring unnecessary fees. But I leave it up to you.

HeinrichApfelmus · 2024-04-25T13:08:50Z

lib/unit/test/unit/Cardano/Wallet/DelegationSpec.hs

@@ -207,21 +263,21 @@ prop_guardJoinQuit knownPoolsList dlg pid wdrl mRetirementInfo = checkCoverage
            pure $ W.currentEpoch info >= W.retirementEpoch info

 prop_guardQuitJoin
-    :: NonEmptyList PoolId
+    :: (Set PoolId -> WalletDelegation -> PoolId -> Maybe PoolRetirementEpochInfo -> Maybe Bool -> Either ErrCannotJoin ())


Suggested change

:: (Set PoolId -> WalletDelegation -> PoolId -> Maybe PoolRetirementEpochInfo -> Maybe Bool -> Either ErrCannotJoin ())

:: GuardQuitJoinFun

A type synonym

type GuardQuitJoinFun = Set PoolId -> WalletDelegation -> PoolId -> Maybe PoolRetirementEpochInfo -> Maybe Bool -> Either ErrCannotJoin ()

helps with understanding.

HeinrichApfelmus · 2024-04-25T13:11:10Z

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

@@ -117,26 +118,36 @@ joinStakePoolDelegationAction
            W.getPoolRetirementCertificate poolStatus

 guardJoin
-    :: Set PoolId
+    :: Write.IsRecentEra era
+    => Write.RecentEra era


HeinrichApfelmus

Thanks! 😊 I still have a couple of comments

Importantly, I think that there is still a corner case in joinStakePoolAction where a user may be prevented from delegating to a stake pool if they are voting but not delegating to a pool (a state of affairs which has to be brought about by a different wallet). 🤔 I believe that changing the Maybe Bool type to an enumeration type with more speaking constructors (or additional constructors) may help resolve this issue.

HeinrichApfelmus · 2024-04-29T10:16:41Z

lib/wallet/src/Cardano/Wallet/IO/Delegation.hs

+                pure (Nothing, Nothing)
+        optionalDelegationAction <- forM delRequestM $
+            handleDelegationRequest ctx currentEpochSlotting getKnownPools
+            getPoolStatus withdrawal votingSameAgain


Suggested change

getPoolStatus withdrawal votingSameAgain

getPoolStatus withdrawal votingSameAgain

Indentation for clarity.

HeinrichApfelmus · 2024-04-29T10:25:04Z

lib/wallet/src/Cardano/Wallet/IO/Delegation.hs

+    (_,(_, dlg),_) <- W.readWallet ctx
    (_, stakeKeyIsRegistered) <- db & \DBLayer{atomically,walletState} ->


Suggested change

(_,(_, dlg),_) <- W.readWallet ctx

(_, stakeKeyIsRegistered) <- db & \DBLayer{atomically,walletState} ->

(calculateWalletDelegations, stakeKeyIsRegistered) <- db & \DBLayer{atomically,walletState} ->

Could you remove the call to W.readWallet here, and instead inline the computation of dlg from the result of readDelegation, like this:

currentEpochSlotting <- getCurrentEpochSlotting nl let dlg = calculateWalletDelegations currentEpochSlotting

This improves the separation of concerns — modules that are higher in the hierarchy (Cardano.Wallet) should be assembled from children that are lower in the hierarchy (Cardano.Wallet.*), not the other way round.

yes, thanks

HeinrichApfelmus · 2024-04-29T10:36:14Z

lib/wallet/src/Cardano/Wallet/IO/Delegation.hs

+    (_,(_, dlg),_) <- W.readWallet ctx
+


Suggested change

(_,(_, dlg),_) <- W.readWallet ctx

As in the other comment — for better separation of concerns, could you compute dlg by inlining the call to W.readWallet here? Compared to the situation above, the value currentEpochSlotting is now already in scope.

HeinrichApfelmus · 2024-04-29T10:38:30Z

specifications/api/swagger.yaml

+  properties:
+    message:
+      type: string
+      description: May occur in Conway onwards when the same vote was casted.


Suggested change

description: May occur in Conway onwards when the same vote was casted.

description: May occur in Conway onwards when the same vote was cast.

The past participle is "cast".

HeinrichApfelmus · 2024-04-29T10:38:40Z

specifications/api/swagger.yaml

+  properties:
+    message:
+      type: string
+      description: May occur in Conway onwards when a given poolId matches the current delegation preferences of the wallet's account and also the same vote was casted.


Suggested change

description: May occur in Conway onwards when a given poolId matches the current delegation preferences of the wallet's account and also the same vote was casted.

description: May occur in Conway onwards when a given poolId matches the current delegation preferences of the wallet's account and also the same vote was cast.

HeinrichApfelmus · 2024-04-29T12:04:12Z

lib/api/src/Cardano/Wallet/Api/Http/Shelley/Server.hs

-            Nothing ->
-                pure Nothing
+        (optionalDelegationAction, optionalVoteAction) <- liftIO $
+            IODeleg.handleDelegationVoteRequest wrk


HeinrichApfelmus · 2024-04-29T12:24:07Z

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

    -> WalletDelegation
    -> PoolId
    -> Maybe PoolRetirementEpochInfo
+    -> Maybe Bool


Suggested change

-> Maybe Bool

-> VoteChange

I find the meaning of this Maybe Bool argument difficult to understand ("boolean blindness"). Given the correctness issue in joinStakePool, I would like to have an type with more speaking names here. How about

data VoteChange = Unimportant | NotVotingToVoting | VotingToAny

Hm, actually, I don't now what to put here. 🤔

I think that joinStakePoolDelegationAction uses the following logic:

If the wallet is not delegating, it's ok.

If the wallet is delegating, but not voting, it's ok.

If the wallet is delegating, and already voting — to abstain or to any other DRep —, then guardJoin should disallow the action.

However, for handleDelegationVoteRequest, the logic is different

If the wallet is delegating, and already voting — to abstain or to any other DRep —, then it's ok unless the new vote is exactly the same as the old vote.

If you ask me, the easiest solution is probably to let handleDelegationVoteRequest allow everything, so that constructUnsignedTransaction will always succeed. 😅

added
data VoteRequest = NotVotedYet | VotedSameLikeBefore | VotedDifferently
to address logic refactoring

HeinrichApfelmus · 2024-04-29T12:29:28Z

lib/integration/scenarios/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                }]
+            }|]
+        rTx2 <- request @(ApiConstructTransaction n) ctx
+            (Link.createUnsignedTransaction @'Shelley src) Default delegationJoin


I would argue testing in new tx workflow is also crucial as we want to be consistent here and outlaw some action (those that are redundant and incur fees)

Well in this case, I would argue that a user of createUnsignedTransaction is a programmer and knows what they are doing, whereas joinStakePool is a proxy for actions from Daedalus users. So the latter would benefit from not incurring unnecessary fees. But I leave it up to you.

HeinrichApfelmus · 2024-04-29T14:00:42Z

lib/integration/scenarios/Test/Integration/Scenario/API/Shelley/StakePools.hs

+            waitForTxStatus ctx w InLedger . getResponse
+                =<< joinStakePool @n ctx (SpecificPool pool) (w, fixturePassphrase)
+
+            eventually "Wallet is delegating to pool and voting abstain" $ do


HeinrichApfelmus · 2024-04-29T14:00:57Z

lib/integration/scenarios/Test/Integration/Scenario/API/Shelley/StakePools.hs

+            r <- joinStakePool @n ctx (SpecificPool pool) (w, fixturePassphrase)
+            verify r
+                [ expectResponseCode HTTP.status403
+


Suggested change

(stray newline)

more

code format code format

fix DelegationSpec

HeinrichApfelmus

Looking good now, thank you! 😊 One last request on grammar, though.

HeinrichApfelmus · 2024-04-30T13:45:22Z

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

@@ -75,6 +82,9 @@ data DelegationRequest
    -- ^ Stop delegating if the wallet is delegating.
    deriving (Eq, Show)

+data VoteRequest = NotVotedYet | VotedSameLikeBefore | VotedDifferently


Suggested change

data VoteRequest = NotVotedYet | VotedSameLikeBefore | VotedDifferently

data VoteRequest = NotVotedYet | VotedSameAsBefore | VotedDifferently

This is grammatically better. 😅

…il in a few bullet points the work accomplished in this PR. Before you submit, don't forget to: CODE-OF-CONDUCT.md CONTRIBUTING.md LICENSE MAINTAINERS.md README.md cabal.project configs default.nix docker-compose.yml docs flake.lock flake.nix floskell.json fourmolu.yaml hie-direnv.yaml hie.yaml justfile lib nix prototypes reports scripts shell.nix specifications test touch.me.CI weeder.dhall Make sure the GitHub PR fields are correct: ✓ Set a good Title for your PR. ✓ Assign yourself to the PR. ✓ Assign one or more reviewer(s). ✓ Link to a Jira issue, and/or other GitHub issues or PRs. ✓ In the PR description delete any empty sections and all text commented in  - [x] Change of re-delegation logic. - [x] Adjust unit tests - [x] Add new unit tests - [x] Add integration testing showing the case - [x] Babbage: re-delegation to the same pool -> ERROR - [x] Conway: re-delegation to the same pool -> ERROR - [x] Conway: re-delegation to the same pool + new vote or re-vote other than recent -> OK - [x] Conway: re-delegation to the same pool + vote the same again -> ERROR - [x] Conway: re-delegation to the different pool + vote the same again -> OK - [x] Conway: vote the same again -> ERROR - [x] check joinStakePools - [x] add handleDelegationVoteRequest ### Comments In order to run integration tests do the following: (a) Babbage ``` just integration-tests-cabal-match TRANS_NEW_JOIN_01f ``` (b) Conway ``` just conway-integration-tests-cabal-match TRANS_NEW_JOIN_01f ```  ### Issue Number adp 3312  Source commit: cb43316

paweljakubas requested a review from HeinrichApfelmus April 23, 2024 12:08

paweljakubas self-assigned this Apr 23, 2024

paweljakubas force-pushed the paweljakubas/adp-3312/allow-redelegation-to-the-same-pool branch 3 times, most recently from bc651c6 to 8c31931 Compare April 24, 2024 15:05

HeinrichApfelmus requested changes Apr 25, 2024

View reviewed changes

paweljakubas requested a review from HeinrichApfelmus April 26, 2024 15:34

paweljakubas force-pushed the paweljakubas/adp-3312/allow-redelegation-to-the-same-pool branch from 6901504 to c61c1e8 Compare April 26, 2024 15:46

HeinrichApfelmus reviewed Apr 29, 2024

View reviewed changes

paweljakubas added 21 commits April 30, 2024 12:47

extend guardJoin

ebf8dbe

adjust unit testing - part 1

92e3b43

adjust unit testing - part 2

c552961

integration test - part 1

d8ab7f9

integration test - part 2

4675edf

integration test - part 3

84f3b84

code standards

aac0f3a

finetune error msg

847070c

hlint

e390a0b

detect the same vote in voteAction

abafcdb

change guardJoin along with tests

c667050

adjust integration testing

f64e8f5

outlaw re-voting the same

9548595

further simplification

9d7ef3b

adjust DelegetionSpec

8ee8b12

type alias

9c25565

add tests for joinStakePools

ee257db

fix joinStakePool logic

c5915f0

add handleDelegationVoteRequest

54fa9d2

polishing

bcc9786

more

add another stake pools integration test

0a3c191

paweljakubas force-pushed the paweljakubas/adp-3312/allow-redelegation-to-the-same-pool branch from c61c1e8 to 0a3c191 Compare April 30, 2024 10:48

use readDelegations

7b9d9f2

paweljakubas requested a review from HeinrichApfelmus April 30, 2024 12:55

paweljakubas force-pushed the paweljakubas/adp-3312/allow-redelegation-to-the-same-pool branch from 41c157e to c4f9079 Compare April 30, 2024 12:57

introduce VoteRequest to simplify logic

bd71f61

code format code format

paweljakubas force-pushed the paweljakubas/adp-3312/allow-redelegation-to-the-same-pool branch from c4f9079 to bd71f61 Compare April 30, 2024 13:01

fix DelegationSpec

81059e4

fix DelegationSpec

HeinrichApfelmus approved these changes Apr 30, 2024

View reviewed changes

VotedSameLikeBefore -> VoteSameAsBefore

f7d9712

paweljakubas added this pull request to the merge queue Apr 30, 2024

Merged via the queue into master with commit cb43316 Apr 30, 2024
3 checks passed

paweljakubas deleted the paweljakubas/adp-3312/allow-redelegation-to-the-same-pool branch April 30, 2024 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow redelegation to the same pool in Conway #4562

Allow redelegation to the same pool in Conway #4562

paweljakubas commented Apr 23, 2024 •

edited

Loading

HeinrichApfelmus Apr 25, 2024 •

edited

Loading

paweljakubas Apr 26, 2024

HeinrichApfelmus Apr 25, 2024

paweljakubas Apr 26, 2024

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus Apr 25, 2024

paweljakubas Apr 26, 2024

HeinrichApfelmus Apr 25, 2024

HeinrichApfelmus left a comment

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus Apr 29, 2024

paweljakubas Apr 30, 2024

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus Apr 29, 2024

paweljakubas Apr 30, 2024

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus Apr 29, 2024

HeinrichApfelmus left a comment

HeinrichApfelmus Apr 30, 2024

	:: (Set PoolId -> WalletDelegation -> PoolId -> Maybe PoolRetirementEpochInfo -> Maybe Bool -> Either ErrCannotJoin ())
	:: GuardQuitJoinFun

	getPoolStatus withdrawal votingSameAgain
	getPoolStatus withdrawal votingSameAgain

		(_,(_, dlg),_) <- W.readWallet ctx
		(_, stakeKeyIsRegistered) <- db & \DBLayer{atomically,walletState} ->

	(_,(_, dlg),_) <- W.readWallet ctx
	(_, stakeKeyIsRegistered) <- db & \DBLayer{atomically,walletState} ->
	(calculateWalletDelegations, stakeKeyIsRegistered) <- db & \DBLayer{atomically,walletState} ->

	description: May occur in Conway onwards when the same vote was casted.
	description: May occur in Conway onwards when the same vote was cast.

	data VoteRequest = NotVotedYet \| VotedSameLikeBefore \| VotedDifferently
	data VoteRequest = NotVotedYet \| VotedSameAsBefore \| VotedDifferently

Allow redelegation to the same pool in Conway #4562

Allow redelegation to the same pool in Conway #4562

Conversation

paweljakubas commented Apr 23, 2024 • edited Loading

Comments

Issue Number

HeinrichApfelmus Apr 25, 2024 • 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

Choose a reason for hiding this comment

HeinrichApfelmus 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

HeinrichApfelmus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Apr 23, 2024 •

edited

Loading

HeinrichApfelmus Apr 25, 2024 •

edited

Loading