Voting WRITE side and api logic

[paweljakubas]

• write side of certs (from wallet to ledger)
• logic - is conway era
• logic for withdrawals in conway
• vote to abstain logic

Comments

Issue Number

adp-3212

[HeinrichApfelmus]

Thank you for the pull request!

First round of comments.

The most important request is that I do not wish the "delegate to abstain" logic be part of the constructTransaction endpoint — I want it to be part of the joinStakePool endpoint.

[HeinrichApfelmus]

Suggested change

	checkingIfVoted
	:: DBLayer IO s
	-> NetworkLayer IO block
	-> ExceptT ErrConstructTx IO ()
	checkingIfVoted DBLayer{..} nw = do
	era <- liftIO $ currentNodeEra nw
	case era of
	Cardano.AnyCardanoEra Cardano.ConwayEra -> do
	voted <- liftIO $ atomically (alreadyVoted walletState)
	if voted then
	pure ()
	else
	throwE ErrConstructTxWithdrawalWithoutVoting
	_ -> pure ()
	alreadyVoted
	:: Functor stm
	=> DBVar stm (DeltaWalletState s)
	-> stm Bool
	alreadyVoted walletState =
	Dlgs.isVoting . view #delegations
	<$> readDBVar walletState
	assertIsVoting
	:: DBLayer IO s
	-> RecentEra era
	-> ExceptT ErrConstructTx IO ()
	assertIsVoting DBLayer{..} = \case
	RecentEraConway -> do
	voting <- liftIO . atomically . fmap isVoting $ readDBVar walletState
	if voting then
	pure ()
	else
	throwE ErrConstructTxWithdrawalWithoutVoting
	_ -> pure ()
	isVoting :: WalletState -> Bool
	isVoting = Dlgs.isVoting . view #delegations

[paweljakubas]

done

[HeinrichApfelmus]

Suggested change

	Nothing ->
	liftIO $ W.handleVotingWhenMissingInConway nl db
	(isJust (body ^. #delegations))
	Nothing -> pure Nothing

⚠️

I have come to the conclusion that the "Vote to abstain" feature is best implemented as part of the joinStakePool endpoint. The reason is that

• The intent of constructTransaction is to construct a transaction from the given JSON data. It performs a bit of validation, but the JSON input is essentially a one-to-one representation of a Cardano transaction — no "magic".
• The intent of joinStakePool function is to perform the necessary steps to join a stake pool — this includes "magic" such as automatically voting to a DRep.

---

In general, the Cardano.Wallet.Api.Http.Shelley.Server module is not a good place for business logic such as handleVotingWhenMissingInConway. The main concern of this module is to parse and partially validate JSON data. We need to separate concerns properly in order to keep our code manageable. I'm afraid, but this module has grown way past manageable — and this slows down development of new features and dependency updates.

[paweljakubas]

When it comes to constructTransaction endpoint this is part of NEW transaction flow where we have construct/sign/decode/submit that contains all new features that were are in the last two years. joinStakePool like postTransaction are legacy endpoints (they have construct/sign/submit in one step). Of course I understand that we might ALSO support joinStakePool having conway voting feature as daedalus might use it (although not sure if they have not adopted new tx workflow). Nevertheless, I do think voting should be primarly supported in constructTransaction....

[HeinrichApfelmus]

Nevertheless, I do think voting should be primarly supported in constructTransaction....

Yes, I do agree with that — what I'm saying is that in constructTransaction, voting should be explicit rather than automatic/implicit. In other words, constructTransaction should not implicitly vote Abstain if the JSON data does not explicitly contain a vote to abstain.

Of course I understand that we might ALSO support joinStakePool having conway voting feature as daedalus might use it (although not sure if they have not adopted new tx workflow).

Yes. I believe that Daedalus is still using joinStakePool, but worth checking.

[HeinrichApfelmus]

Suggested change

	-> Maybe DelegationAction
	-- ^ Delegation action that we plan to take
	-> Maybe VotingAction
	-- ^ Optional vote action in Conway era onwards
	-> DelegationAction
	-- ^ Delegation action that we plan to take

Hm, this function is becoming a mess, I am afraid. 😵‍💫 @paweljakubas, could you:

• remove the logic for Maybe VotingAction from this function again
• and instead add a new module Cardano.Wallet.Transaction.Voting with an analogous function certificateFromVotingAction. Then, you can use this function to create certificates that are specific to voting and join them with the certificates for delegation — see other comment.

[paweljakubas]

yeah, my bad 🥲 I have added Delegation.Voting module

[HeinrichApfelmus]

Suggested change

	let certs = case stakeCred of
	let delegationCerts = case stakeCred of …

            let deduplicateRegister (JoinRegisteringKey k) (VotingRegisteringKey v) = Voting v
                deduplicateRegister _ v = v
            let votingCerts = case deduplicateRegister <$> (#txVotingAction ctx) of … ⟨ certificateFromVotingAction ⟩
            let certs = delegationCerts ++ votingCerts 

Here, the idea is to separate two concerns: Adding delegation certificates and adding voting certificates. It's ok to use two certificates in a single transaction; the code becomes too much of a mess otherwise.

[paweljakubas]

done. I do think nub $ delegCerts <> votingCerts should be enough as we use the same staking key during registration

[HeinrichApfelmus]

I do think nub $ delegCerts <> votingCerts should be enough as we use the same staking key during registration

Ah, nub is a good idea.

One small hitch: We have to careful whether the certificates are applied left-to-right, or right-to-left. In the situation

  nub ["Delegate", "Register", "Vote", "Register"]
=
  nub ["Delegate", "Register", "Vote"] 

applying right-to-left would be a mistake, because nub takes the leftmost element when deduplicating.

[paweljakubas]

@HeinrichApfelmus joinStakePool and quitStakePool logic of vote to abstain added and removed from constructTransaction -> e2627af

[HeinrichApfelmus]

Suggested change

	-> Bool
	-> IO (Maybe VotingAction)
	handleVotingWhenMissingInConway era db delegationsPresent = do
	areWeInConway <- case votingEnabledInEra era of
	Left _ -> pure False
	Right _ -> pure True
	voting <- haveWeVoted db
	stakingKeyRegistered <- isStakeKeyInDb db
	if (areWeInConway && not voting && delegationsPresent) then
	-> IO (Maybe VotingAction)
	handleVotingWhenMissingInConway era db = do
	areWeInConway <- case votingEnabledInEra era of
	Left _ -> pure False
	Right _ -> pure True
	voting <- haveWeVoted db
	stakingKeyRegistered <- isStakeKeyInDb db
	if (areWeInConway && not voting) then

After removing the handleVotingWhenMissingInConway from constructTransaction and making it part of selectCoinsForJoin, the delegationsPresent parameter is now always True — and we can remove it. 🤓

[HeinrichApfelmus]

Suggested change

	optionalVoteAction <-
	liftIO $ W.handleVotingWhenMissingInConway era db True
	let txCtx = defaultTransactionCtx
	{ txDelegationAction = Just action
	, txWithdrawal = withdrawal
	, txVotingAction = optionalVoteAction
	let txCtx = defaultTransactionCtx
	{ txDelegationAction = Just action
	, txWithdrawal = withdrawal

We do not need to supply a txVotingAction ≠ Nothing here, because the result of WD.quitStakePoolDelegationAction can only be Quit. There is no point in adding a vote to Abstain if we want to quit anyway. 🤔

We do need the txDeposit, however — the code that constructs the certificate corresponding to the Quit action will refuse to do so without a deposit size. 😲

[paweljakubas]

indeed, txDeposit is needed when registration and derigistration happens. no optionalVoteAction during quitting stake pool!

[HeinrichApfelmus]

I do think when quitPools happens and there is voting then we need to emit user error ....

As the "Quit" endpoint goes, I think it's better to not emit an error when the wallet is voting, because this is a legacy endpoint and doesn't expect anything about voting anyway.

As far as this function goes, I think this would not happen anyway? 🤔

• handleVotingWhenMissingInConway doesn't emit an error.
• The only reason why there may be an error is when txDeposit = Nothing and txDelegationAction = Just Quit and the function certificateFromDelegationAction will hit the following case:

       (Quit, Nothing) ->
           error "certificateFromDelegationAction: deposit value required in \
                 \Conway era when registration is carried out (quitting)"

I would like to not hit that case, because that's a bug — we didn't provide the deposit value.

[Anviking]

txDeposit is needed when registration and derigistration happens

Related: https://cardanofoundation.atlassian.net/browse/ADP-3270

To properly handle:

-- | De-Register the staking credential. Deposit, if present, must match the amount
-- that was left as a deposit upon stake credential registration.

https://github.com/IntersectMBO/cardano-ledger/blob/53a64c898408966940c66fa4b29d42f2656240f4/eras/conway/impl/src/Cardano/Ledger/Conway/TxCert.hs#L391-L392

i.e., the deposit not necessarily being currentPP ^. ppKeyDepositL, I believe we'd need to either get it over LSQ if possible or track it in our DB.

[HeinrichApfelmus]

track it in our DB.

😵‍💫 I think that's what we have to do — but perhaps not as part of the Conway hardfork.

The ledger still recognizes the old way of unregistering certificates, it's just that Cardano.Api does not expose it as part of makeStakeAddressUnregistrationCertificate. It's possible to use the constructor ConwayCertificate directly, though.

[HeinrichApfelmus]

    -> Coin
        -- ^ Deposit value

Ideally, we want this to be Coin rather than Maybe Coin — the deposit is always known from the protocol parameters. However, fixing this in the current codebase is hard — the best approach would be to carry around the protocol parameters around all the way down mkUnsignedTransaction or even further.

[paweljakubas]

I will leave it for separate PR

[HeinrichApfelmus]

Suggested change

	( CardanoApiEra
	, RecentEra (RecentEraBabbage, RecentEraConway)
	)

(🤷 I see no value in adding explicit exports to qualified imports.)

[paweljakubas]

yep, the same was in Transaction.Delegation module and I automatically copy pasted it. Now removed that strange way of importing in both modules

[HeinrichApfelmus]

👍 I have checked. In Conway, the rule handling the certificates is called CERTS. As far as I can tell, it goes left to right, so nub is appropriate here.