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

Eliminate Boolean properties #649

Merged
merged 10 commits into from
Jan 31, 2025
Merged

Conversation

carlostome
Copy link
Contributor

@carlostome carlostome commented Jan 23, 2025

Description

Addresses #342.

Original:

  • actionWellFormed
  • isP2Script
  • isAdaOnlyᵇ
  • feesOK

Other:

  • isTwoPhaseScriptAddress

Left:

  • isValid

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Any semantic changes to the specifications are documented in CHANGELOG.md
  • Code is formatted according to CONTRIBUTING.md
  • Self-reviewed the diff

@carlostome carlostome linked an issue Jan 23, 2025 that may be closed by this pull request
4 tasks
@carlostome carlostome force-pushed the 342-eliminate-boolean-properties branch from cf59b69 to f730932 Compare January 23, 2025 12:40
@carlostome carlostome marked this pull request as ready for review January 24, 2025 12:57
Copy link
Collaborator

@WhatisRT WhatisRT left a comment

Choose a reason for hiding this comment

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

feesOK looks great! The other two still need a bit of work.

src/Ledger/Transaction.lagda Outdated Show resolved Hide resolved
src/Ledger/Transaction.lagda Outdated Show resolved Hide resolved
(filterˢ (λ (a , _ ) → isTwoPhaseScriptAddress tx utxo a ≡ true)
(range (utxo ∣ txins)))
(filterˢ (λ (a , _ ) → isTwoPhaseScriptAddress tx utxo a)
⦃ λ {(a , _)} → isTwoPhaseScriptAddress? {tx} {utxo} {a} ⦄
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we have an interesting problem: this code belongs to the visible section of the PDF and we shouldn't show stuff like that because we'd have to explain what it means (and the meaning isn't particularly interesting). However, marking isTwoPhaseScriptAddress? as an instance isn't a good idea and wouldn't even work anyway.

I think the only reasonable solution here is to turn isTwoPhaseScriptAddress into a data type and then provide a instance for that type. As I see it, there are two options to do that:

  • either completely replace the function with a type whose constructors contain the same logic, or
  • provide a 'newtype' wrapper around isTwoPhaseScriptAddress, i.e. something of this form:
    data isTwoPhaseScriptAddress' : Tx → UTxO → Addr → Type where
      wrap : ∀ {tx utxo a} → isTwoPhaseScriptAddress tx utxo a → isTwoPhaseScriptAddress' tx utxo a
    

The second option has the upside that we can cheat a little and just hide this wrapper behind the curtain when rendering the PDF. We currently don't have the machinery to do that perfectly because we'd have to substitute isTwoPhaseScriptAddress' with isTwoPhaseScriptAddress in the line above when rendering the PDF, but we probably need a mechanism for that anyway. So I'm currently leaning more towards that option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would we go about substituting isTwoPhaseScriptAddress' by its unprimed version in the pdf?

@WhatisRT
Copy link
Collaborator

@carlostome I'd like to merge this, the only thing that's still open is the choice of Dec constructors. If you have a reason to prefer _because_ we can leave it as is, but otherwise I'd prefer to go with yes and no for consistency.

@carlostome carlostome requested a review from WhatisRT January 31, 2025 13:30
Copy link
Collaborator

@WhatisRT WhatisRT left a comment

Choose a reason for hiding this comment

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

Thanks! I've renamed isAdaOnlyᵇ to isAdaOnly because the was just to indicate that it's a boolean. I've also removed a ˢ that wasn't necessary anymore.

I don't want to investigate that right now, can be done some other time.
@WhatisRT
Copy link
Collaborator

Whoa, apparently removing the ˢ breaks something in a completely different place. No idea what's going on there, I'll just revert that for now.

@WhatisRT WhatisRT merged commit 2566862 into master Jan 31, 2025
8 of 10 checks passed
@WhatisRT WhatisRT deleted the 342-eliminate-boolean-properties branch January 31, 2025 14:46
github-actions bot added a commit that referenced this pull request Jan 31, 2025
@WhatisRT WhatisRT mentioned this pull request Jan 31, 2025
4 tasks
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.

Eliminate boolean properties
2 participants