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 TxProposalProcedures unsafe constructor with an unidirectional pattern #726

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Jan 14, 2025

Changelog

- description: |
    Replace `TxProposalProcedures` unsafe constructor with an unidirectional pattern. Use `mkTxProposalProcedures` for creating the type instead.
# 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
   - refactoring    # QoL changes
  # - 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

Previously TxProposalProcedures was not guaranteeing the type consistency: when used, you could insert proposals only to its second argument without inserting it to the first. The logic in cardano-api relies on a fact that the first argument would contain a complete list of proposals, so this would introduce a bug.

This PR replaces TxProposalProcedures constructor with an unidirectional pattern, which does not allow constructing of a type. mkTxProposalProcedures should be used for that.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@carbolymer carbolymer self-assigned this Jan 29, 2025
@carbolymer carbolymer force-pushed the mgalazyn/refactor/replace-txproposalprocedure-with-pattern branch 4 times, most recently from 8c7adb6 to 3193e8c Compare February 4, 2025 10:52
@carbolymer carbolymer changed the title Replace TxProposalProcedure with a pattern Replace TxProposalProcedure with constructor a pattern Feb 4, 2025
@carbolymer carbolymer changed the title Replace TxProposalProcedure with constructor a pattern Replace TxProposalProcedure with constructor an unidirectional pattern Feb 4, 2025
@carbolymer carbolymer changed the title Replace TxProposalProcedure with constructor an unidirectional pattern Replace TxProposalProcedures unsafe constructor with an unidirectional pattern Feb 4, 2025
@carbolymer carbolymer marked this pull request as ready for review February 4, 2025 10:57
@carbolymer carbolymer force-pushed the mgalazyn/refactor/replace-txproposalprocedure-with-pattern branch from 3193e8c to 1616394 Compare February 4, 2025 11:02
@@ -1557,13 +1564,19 @@ substituteExecutionUnits
]

substitutedExecutionUnits <- traverseScriptWitnesses eSubstitutedExecutionUnits
-- join again with osetProposalProcedures, just in case anything from there was not included in substituteExecutionUnits
Copy link
Contributor

Choose a reason for hiding this comment

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

We should either know or not know if everything was included in substituteExecutionUnits.

@@ -1471,7 +1472,7 @@ data TxProposalProcedures build era where
TxProposalProceduresNone :: TxProposalProcedures build era
-- | Create Tx proposal procedures. Prefer 'mkTxProposalProcedures' smart constructor to using this constructor
-- directly.
TxProposalProcedures
UnsafeTxProposalProcedures
Copy link
Contributor

@Jimbo4350 Jimbo4350 Feb 4, 2025

Choose a reason for hiding this comment

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

We should change this to:

TxProposalProcedures
    :: Ledger.EraPParams (ShelleyLedgerEra era)
    =>  BuildTxWith build (Map (L.ProposalProcedure (ShelleyLedgerEra era)) (Maybe (ScriptWitness WitCtxStake era)))
    -> TxProposalProcedures build era

Then we can avoid having a separate OSet. Any ProposalProcedure with a corresponding Nothing is key does not require a script witness.

Copy link
Contributor Author

@carbolymer carbolymer Feb 4, 2025

Choose a reason for hiding this comment

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

But then conversion from ledger tx body to our tx body will result in our datatype having no proposal procedures, because build ~ ViewTx.

Ideally it should be something like (similar to TxCertificates):

TxProposalProcedures
    :: Ledger.EraPParams (ShelleyLedgerEra era)
    => OMap 
         (L.ProposalProcedure (ShelleyLedgerEra era)) 
         (BuildTxWith build (Maybe(ScriptWitness WitCtxStake era)))
    -> TxProposalProcedures build era

But it was rejected on a review, when I was making a first fix to TxProposalProcedures issue with missing proposals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's go with your counter suggestion 👍

We need to remove BuildTxWith at some point as it's not a useful abstraction.

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.

2 participants