[ADP-3212] Add voting certificates to primitive lib #4427

paolino · 2024-02-01T18:21:09Z

Add Conway certificates support to primitive lib

All credits to Pawel

paweljakubas

of course from my point of view everything is good here. But would be good for @HeinrichApfelmus to do his due dilligence :-)

HeinrichApfelmus

Thanks for the split PR. 😊 Much easier to review.

@paweljakubas Could you rename the types to DRep and DRepID as I describe in the comments? I believe it's important that we use the exact same naming conventions as in the specification in order to avoid cognitive overhead — whenever we see the type DRep in our code, we can be sure that it's the same DRep as mentioned in the specification, and some homegrown variant of it that may or may not work.

lib/primitive/lib/Cardano/Wallet/Primitive/Types/DRep.hs

lib/primitive/lib/Cardano/Wallet/Primitive/Ledger/Read/Tx/Features/Certificates.hs

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Certificates.hs

HeinrichApfelmus

Another request concerning the DelegationCertificate type.

HeinrichApfelmus · 2024-02-02T16:01:32Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Certificates.hs

@@ -76,6 +80,8 @@ data DelegationCertificate
    = CertDelegateNone RewardAccount
    | CertDelegateFull RewardAccount PoolId
    | CertRegisterKey RewardAccount
+    | CertVoteFull RewardAccount VoteAction
+    | CertDelegateAndVoteFull RewardAccount PoolId VoteAction


Suggested change

| CertDelegateAndVoteFull RewardAccount PoolId VoteAction

| CertVoteAndDelegate RewardAccount (Maybe DRep) (Maybe PoolId)

The formal specification for Conway uses the following constructor for the DCert state transition datum:

Here, both voting and delegation are joined into a single constructor.

Again, in order to stick to existing naming conventions, I would follow their example here.

Using a single constructor is likely to slightly simplify our code. We could remove the CertDelegateFull constructor as well, but that would involve changes to our code that are beyond the scope of this pull request.

used CertVoteAndDelegate and removed other two that are handled here after adding optional pool and vote

Nice. 👍 I originally thought that CertVoteFull had a wider impact on the code, but nice to see that it's blast radius is very limited.

lib/wallet/src/Cardano/Wallet/DB/Pure/Implementation.hs

lib/wallet/src/Cardano/Wallet/DB/Store/Delegations/Layer.hs

HeinrichApfelmus · 2024-02-02T16:03:58Z

I think the name changes are important in order to reduce cognitive load when comparing the specification to our code.

Other than these changes, I'm happy to approve the pull request. 👍

HeinrichApfelmus

Thanks! 😊 Only minor comments. I recommend to inline the definition of ApiDRep in the swagger API specification again.

HeinrichApfelmus · 2024-02-05T13:01:24Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/DRep.hs

+                        Right $ VoteTo $ DRepFromScriptHash scripthash
+                    Left _ -> Left $ TextDecodingError $ unwords
+                        [ "I couldn't parse the given vote action."
+                        , "I am expecting either 'abstain', 'no confidence'"


slipped away 😅

lib/primitive/lib/Cardano/Wallet/Primitive/Types/DRep.hs

HeinrichApfelmus · 2024-02-05T13:07:58Z

specifications/api/swagger.yaml

+x-noVote: &noVote
+  type: string
+  enum:
+    - abstain
+    - no_confidence
+
+x-anyVoting: &anyVoting
  nullable: false
  oneOf:
    - <<: *drepKeyHash
+      title: vote to a drep represented by key hash
    - <<: *drepScriptHash
+      title: vote to a drep represented by script hash
+    - <<: *noVote
+      title: casting no vote


Suggested change

x-noVote: &noVote

type: string

enum:

- abstain

- no_confidence

x-anyVoting: &anyVoting

nullable: false

oneOf:

- <<: *drepKeyHash

title: vote to a drep represented by key hash

- <<: *drepScriptHash

title: vote to a drep represented by script hash

- <<: *noVote

title: casting no vote

I recommend that we mirror the definition of DRep from the specification (and now from the code as well) in the ApiDRep type, i.e. inline everything there.

HeinrichApfelmus · 2024-02-05T13:14:13Z

specifications/api/swagger.yaml

@@ -3717,18 +3729,13 @@ components:
            asset_name: *assetName
            operation: *ApiMintBurnOperation

-    ApiVoteAction: &ApiVoteAction
+    ApiDRep: &ApiDRep
+      <<: *anyVoting


Suggested change

<<: *anyVoting

good suggestion, thanks 💯

HeinrichApfelmus · 2024-02-05T13:16:10Z

specifications/api/swagger.yaml

+        Voting can be done together with delegation action or as a standalone action.
+      type: string


Suggested change

Voting can be done together with delegation action or as a standalone action.

type: string

ApiDRep: &ApiDRep

description: |

Decentralized representative (DRep)

that the wallet is delegating its vote to.

One can abstain, give no confidence vote,

or vote for a representative specified by a key hash or script hash.

Vote delegation can be done together with stake delegation action.

nullable: false

type: string

oneOf:

- enum:

- abstain

- no_confidence

title: casting a default vote

- <<: *drepKeyHash

title: vote to a drep represented by key hash

- <<: *drepScriptHash

title: vote to a drep represented by script hash

Inlined, and amended the formulation slightly.

HeinrichApfelmus · 2024-02-05T13:18:10Z

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Certificates.hs

@@ -76,6 +80,8 @@ data DelegationCertificate
    = CertDelegateNone RewardAccount
    | CertDelegateFull RewardAccount PoolId
    | CertRegisterKey RewardAccount
+    | CertVoteFull RewardAccount VoteAction
+    | CertDelegateAndVoteFull RewardAccount PoolId VoteAction


Nice. 👍 I originally thought that CertVoteFull had a wider impact on the code, but nice to see that it's blast radius is very limited.

…x] Add Conway certificates support to primitive lib ADP-3212 All credits to Pawel Source commit: 30dc6ab

paolino self-assigned this Feb 1, 2024

paolino requested review from paweljakubas and HeinrichApfelmus February 1, 2024 18:21

paolino force-pushed the paweljakubas/ADP-3212/voting-primitives branch from f8b7a65 to cda730c Compare February 1, 2024 18:55

paweljakubas approved these changes Feb 2, 2024

View reviewed changes

paolino force-pushed the paweljakubas/ADP-3212/voting-primitives branch 2 times, most recently from d22b1f2 to 5f1bc71 Compare February 2, 2024 14:04

HeinrichApfelmus reviewed Feb 2, 2024

View reviewed changes

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Certificates.hs Outdated Show resolved Hide resolved

HeinrichApfelmus reviewed Feb 2, 2024

View reviewed changes

lib/primitive/lib/Cardano/Wallet/Primitive/Types/Certificates.hs Outdated Show resolved Hide resolved

HeinrichApfelmus reviewed Feb 2, 2024

View reviewed changes

lib/wallet/src/Cardano/Wallet/DB/Pure/Implementation.hs Outdated Show resolved Hide resolved

HeinrichApfelmus reviewed Feb 2, 2024

View reviewed changes

lib/wallet/src/Cardano/Wallet/DB/Store/Delegations/Layer.hs Outdated Show resolved Hide resolved

paolino and others added 7 commits February 5, 2024 13:26

Add DRep module

d63982e

Add Conway certificates to Types

42fdef7

Add Conway certificates to Read features

af96842

Add bottom implementations for wallet code on Conway certificates

06be740

apply review suggestions

e9e96f0

golden regeneration and enhancing unit tests

bfe643b

add missing cert to fromConwayCerts

b3fc551

paweljakubas force-pushed the paweljakubas/ADP-3212/voting-primitives branch from 5f1bc71 to b3fc551 Compare February 5, 2024 12:27

paweljakubas requested a review from HeinrichApfelmus February 5, 2024 12:31

hlint/stylish

467d0d0

HeinrichApfelmus approved these changes Feb 5, 2024

View reviewed changes

final polishing

6a9e373

paweljakubas added this pull request to the merge queue Feb 5, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 5, 2024

paweljakubas added this pull request to the merge queue Feb 5, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 5, 2024

paweljakubas added this pull request to the merge queue Feb 5, 2024

Merged via the queue into master with commit 30dc6ab Feb 5, 2024
3 checks passed

paweljakubas deleted the paweljakubas/ADP-3212/voting-primitives branch February 5, 2024 16:51

WilliamKingNoel-Bot pushed a commit that referenced this pull request Feb 5, 2024

docs: [ADP-3212] Add voting certificates to primitive lib (#4427) - […

8ce3b88

…x] Add Conway certificates support to primitive lib ADP-3212 All credits to Pawel Source commit: 30dc6ab

paolino mentioned this pull request Feb 9, 2024

[ADP-3212] Add Conway certificates reading in wallet lib #4425

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADP-3212] Add voting certificates to primitive lib #4427

[ADP-3212] Add voting certificates to primitive lib #4427

paolino commented Feb 1, 2024

paweljakubas left a comment

HeinrichApfelmus left a comment

HeinrichApfelmus left a comment

HeinrichApfelmus Feb 2, 2024

paweljakubas Feb 5, 2024

HeinrichApfelmus Feb 5, 2024

HeinrichApfelmus commented Feb 2, 2024

HeinrichApfelmus left a comment

HeinrichApfelmus Feb 5, 2024

HeinrichApfelmus Feb 5, 2024

HeinrichApfelmus Feb 5, 2024

paweljakubas Feb 5, 2024

HeinrichApfelmus Feb 5, 2024

paweljakubas Feb 5, 2024

HeinrichApfelmus Feb 5, 2024

	\| CertDelegateAndVoteFull RewardAccount PoolId VoteAction
	\| CertVoteAndDelegate RewardAccount (Maybe DRep) (Maybe PoolId)

		Voting can be done together with delegation action or as a standalone action.
		type: string

[ADP-3212] Add voting certificates to primitive lib #4427

[ADP-3212] Add voting certificates to primitive lib #4427

Conversation

paolino commented Feb 1, 2024

paweljakubas left a comment

Choose a reason for hiding this comment

HeinrichApfelmus left a 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

HeinrichApfelmus commented Feb 2, 2024

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