Logic changes to join/quitStakePools #4468
Conversation
paweljakubas
force-pushed
the
paweljakubas/adp-3261/voting-delegation-withdrawals
branch
from
d9683d3
to
72983cb
Compare
paweljakubas
requested review from
Anviking,
HeinrichApfelmus,
paolino and
jonathanknowles
paweljakubas
force-pushed
the
paweljakubas/adp-3261/voting-delegation-withdrawals
branch
from
88be4b1
to
30818cd
Compare
There was a problem hiding this comment.
Thank you! The integration tests look good to me, especially concerning the use of getTransaction.
That said, in the future, I would like to invest some effort into making the tests easier to understand and maintain — at the moment, separating test intent from programmatic details is hard. This would involve: Not using the Link module, exceptions instead of checking for 202 explicitly, encapsulating getTransaction. But for the purpose of testing functionality, this is good enough for now.
I would like to refactor the changes to the program logic, especially relating to the inConway boolean. For that purpose, @paweljakubas , could you split the pull request into two pull requests:
- A pull request that only contains the changes to the program logic.
- A pull request that uses pull request 1 as base and only contains the tests.
Thanks!
paweljakubas
force-pushed
the
paweljakubas/adp-3261/voting-delegation-withdrawals
branch
from
b6cdfaa
to
bc7b294
Compare
paweljakubas
changed the title
|
@HeinrichApfelmus I slimmed down PR and not it contains logic changes to legacy join/quit+integration tests adjustment that are equired. Plus some small other logic changes |
| @@ -767,7 +767,7 @@ mkUnsignedTransaction networkId stakeCred ctx selection = do | |||
| , "either xpub or script when there is delegation" | |||
| , "action" | |||
| ] | |||
| let certs = L.nub $ votingCerts <> delegCerts | |||
| let certs = L.nub $ delegCerts <> votingCerts | |||
There was a problem hiding this comment.
This one is needed as Registration cert needs to be FIRST...Otherwise node can protest
There was a problem hiding this comment.
I don't quite understand what's going on, because I would expect both votingCerts and delegCerts to contain the initial registration certificate. 🤔 That's actually my main reason for starting the refactoring.
There was a problem hiding this comment.
hmmm....I found one case where this was crucial and got node error....but it was without your refactoring (ie., before I rebased aginst your PR). Anyway, it does not change the picture if we have votingCerts <> delegCerts or delegCerts <> votingCerts.
There was a problem hiding this comment.
I found one case where this was crucial and got node error.
Yes, I believe you — I just don't understand why, and that makes me suspicious that there could be another logic bug in the code. 🤔
There was a problem hiding this comment.
Found it:
joinStakePoolDelegationAction must return Tx.VoteRegisteringKey instead of Tx.Vote in the voting action in order to for the second definition of certs to work when the key is not registered yet. I have fixed it.
| Left _ -> pure False | ||
| Right _ -> pure True | ||
|
|
||
| (action,_) <- WD.joinStakePoolDelegationAction @s |
There was a problem hiding this comment.
🤔 Shouldn't the voting certificate also be part of the transaction that we select coins for?
There was a problem hiding this comment.
There was a problem hiding this comment.
So this endpoint is used by Daedalus for hardware wallet support.
If we want voting supported there, we should ensure the certificate is accounted for in the coin-selection, yes, but also that it is returned in the response. And then Daedalus would need to be updated to add that certificate to the tx.
There was a problem hiding this comment.
And then Daedalus would need to be updated to add that certificate to the tx.
Dang. We can select the coins for it, but we don't assemble the transaction — it looks like Daedalus can't even vote to abstain in the presence of a hardware wallet.
There was a problem hiding this comment.
@paweljakubas Thanks for splitting!
Could you rebase the logic on #4477 ? I have added additional comments — I would like to obtain a strict separation between IO (reading data from mutable sources) and pure functions (performing computation on that data).
| :: WalletDelegation | ||
| -> Withdrawal | ||
| -> Coin | ||
| -> Bool |
There was a problem hiding this comment.
| -> Bool |
Isn't this piece of information contained in the WalletDelegation argument? 🤔
voting = case last_ of
Voting{} → True
DelegatingVoting{} → True
_ → FalseThere was a problem hiding this comment.
Probably, but let's leave it like this for now — the argument is localized to the Cardano.Wallet.Delegation module now.
paweljakubas
force-pushed
the
paweljakubas/adp-3261/voting-delegation-withdrawals
branch
from
a772cbd
to
bcdbdf5
Compare
|
one more commit soon |
HeinrichApfelmus
force-pushed
the
paweljakubas/adp-3261/voting-delegation-withdrawals
branch
from
eaacc57
to
49468a1
Compare
<!--
Detail in a few bullet points the work accomplished in this PR.
Before you submit, don't forget to:
* 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 <!--, so that this text does not appear
in merge commit messages.
* Don't waste reviewers' time:
✓ If it's a draft, select the Create Draft PR option.
✓ Self-review your changes to make sure nothing unexpected slipped
through.
* Try to make your intent clear:
✓ Write a good Description that explains what this PR is meant to do.
✓ Jira will detect and link to this PR once created, but you can also
link this PR in the description of the corresponding Jira ticket.
✓ Highlight what Testing you have done.
✓ Acknowledge any changes required to the Documentation.
-->
- [x] Voting/revoting after delegation
- [x] Delegation/re-delegation after voting
- [x] Delegation/quitting after voting
- [x] Delegation together with voting
- [x] Quitting after voting
- [x] Withdrawals after voting
- [x] Withdrawals before voting
### Comments
<!-- Additional comments, links, or screenshots to attach, if any. -->
### Issue Number
adp-3261
<!-- Reference the Jira/GitHub issue that this PR relates to, and which
requirements it tackles.
Note: Jira issues of the form ADP- will be auto-linked. -->
paweljakubas
deleted the
paweljakubas/adp-3261/voting-delegation-withdrawals
branch
Comments
In order to run
Issue Number
ADP-3261