Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace unsafeMergeVotingProcedures by mergeVotingProcedures #498

Merged
merged 1 commit into from
Mar 28, 2024
+32 −14
Merged

Replace unsafeMergeVotingProcedures by mergeVotingProcedures #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 31 additions & 13 deletions cardano-api/internal/Cardano/Api/Governance/Actions/VotingProcedure.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ import qualified Cardano.Ledger.Api as L
import Cardano.Ledger.Core (EraCrypto)
import qualified Cardano.Ledger.Core as L

import Control.Monad (foldM)
import qualified Data.Map as Map
import qualified Data.Set as Set
import Data.Text (Text)
import qualified Data.Text.Encoding as Text
import GHC.Generics
Expand Down Expand Up @@ -154,16 +156,32 @@ singletonVotingProcedures _ voter govActionId votingProcedure =
$ Map.singleton voter
$ Map.singleton govActionId votingProcedure

-- | Right biased merge of Voting procedures.
-- TODO Conway we need an alternative version of this function that can report conflicts as it is
-- not safe to just throw away votes.
unsafeMergeVotingProcedures :: ()
=> VotingProcedures era
-> VotingProcedures era
-> VotingProcedures era
unsafeMergeVotingProcedures vpsa vpsb =
VotingProcedures
$ L.VotingProcedures
$ Map.unionWith (Map.unionWith const)
(L.unVotingProcedures (unVotingProcedures vpsa))
(L.unVotingProcedures (unVotingProcedures vpsb))
-- | A voter, and the conflicting votes of this voter (i.e. votes with the same governance action identifier)
newtype VotesMergingConflict era =
VotesMergingConflict
( L.Voter (L.EraCrypto (ShelleyLedgerEra era))
, [L.GovActionId (L.EraCrypto (ShelleyLedgerEra era))])

-- | @mergeVotingProcedures vote1 vote2@ merges @vote1@ and @vote2@ into a single vote,
-- or fails if the votes are incompatible.
mergeVotingProcedures :: ()
=> VotingProcedures era -- ^ Votes to merge
-> VotingProcedures era -- ^ Votes to merge
-> Either (VotesMergingConflict era) (VotingProcedures era) -- ^ Either the conflict found, or the merged votes
mergeVotingProcedures vpsa vpsb =
VotingProcedures . L.VotingProcedures <$> foldM mergeVotesOfOneVoter Map.empty allVoters
where
mapa = L.unVotingProcedures (unVotingProcedures vpsa)
mapb = L.unVotingProcedures (unVotingProcedures vpsb)
allVoters = Set.union (Map.keysSet mapa) (Map.keysSet mapb)
mergeVotesOfOneVoter acc voter =
Map.union acc <$> case (Map.lookup voter mapa, Map.lookup voter mapb) of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use differenceWith since all we care about are conflicts? Then we can just do a union on the maps if there are none.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jimbo4350> but we need to report the conflicts. How would we do that with differenceWith, that is not within an error monad/Either-return type?

Copy link
Contributor

@Jimbo4350 Jimbo4350 Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here: ae88ae4

I think we should also property test this function.

Copy link
Contributor Author

@smelc smelc Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also property test this function.

If you're fine with me spending time doing that, yes, 💯 This function has nice properties indeed.

Regarding ae88ae4, sorry I disagree: my version doesn't separate doing the job from checking conflicts, it's doing both at the same time; which is more intuitive, at the cost of very little duplication (only the symmetry of (Just _, Nothing) causes repetition). I rather keep my version for future readers and maintainers of the code (modulo introducing the type synonym which enhances readability, that is neat).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a newtype wrapping the error case and tracked adding properties tests in #499, so I'm enqueueing for merge.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

differenceWith is only able to compare the keys.

If we have to GovActionIds that are equal according to Eq, is that considered a conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@newhoggy> indeed we only compare the keys (GovActionId). So, if there are two equal GovActionId that map to the same VotingProcedures, a conflict will be reported; whereas it's not really a conflict, it's more of a duplication.

(Just v, Nothing) -> Right $ Map.singleton voter v -- Take only available value
(Nothing, Just v) -> Right $ Map.singleton voter v -- Take only available value
(Nothing, Nothing) -> Right Map.empty -- No value
(Just va, Just vb) -> -- Here's the case where we're unioning different votes for the same voter
if null intersection -- No conflict: sets of keys from left and right is disjoint
then Right $ Map.singleton voter (Map.union va vb)
else Left $ VotesMergingConflict (voter, intersection) -- Ooops, a conflict! Let's report it!
where
intersection = Map.keys $ Map.intersection va vb
2 changes: 1 addition & 1 deletion cardano-api/src/Cardano/Api/Shelley.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,8 @@ module Cardano.Api.Shelley
fromLedgerPParamsUpdate,

emptyVotingProcedures,
mergeVotingProcedures,
singletonVotingProcedures,
unsafeMergeVotingProcedures,
) where

import Cardano.Api
Expand Down
Loading