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.

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

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Mar 26, 2024

Changelog

- description: |
    Replace unsafeMergeVotingProcedures by mergeVotingProcedures, that handles incompatible votes and return an error
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - improvement    # QoL changes e.g. refactoring
  - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

How to trust this PR

See IntersectMBO/cardano-cli#681

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Self-reviewed the diff

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

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

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.

@smelc smelc added this pull request to the merge queue Mar 28, 2024
Merged via the queue into main with commit ba43487 Mar 28, 2024
25 checks passed
@smelc smelc deleted the smelc/replace-unsafe-merge-votes branch March 28, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants