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

Remove UnwitnessedCliFormattedTxBody constructor #707

Merged
merged 4 commits into from
May 17, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Apr 9, 2024

Changelog

- description: |
    Remove UnwitnessedCliFormattedTxBody constructor
# 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

Fixes #690

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

@@ -1286,7 +1271,7 @@ runTransactionWitnessCmd
firstExceptT TxCmdWriteFileError . newExceptT
$ writeTxWitnessFileTextEnvelopeCddl sbe outFile witness

UnwitnessedCliFormattedTxBody anyTxbody -> do
ReadTxBody anyTxbody -> do
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of ReadTxBody?

Copy link
Contributor

Choose a reason for hiding this comment

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

So now its only possible to create CDDL formatted transactions. Therefore data IncompleteTx should be a newtype instead. We no longer need two constructors.

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> changed 👍

Can you check this is what you had in mind?

@smelc smelc force-pushed the smelc/remove-UnwitnessedCliFormattedTxBody-constructor-v2 branch from a689d50 to 51cf335 Compare April 15, 2024 09:37
@smelc smelc force-pushed the smelc/remove-UnwitnessedCliFormattedTxBody-constructor-v2 branch from 51cf335 to eb15e23 Compare April 15, 2024 10:01
Comment on lines 534 to 535
return $ Right $ IncompleteCddlTxBody $ inAnyShelleyBasedEra sbe $ getTxBody tx
Right txBody -> return $ Right $ IncompleteCddlTxBody txBody
Copy link
Contributor Author

@smelc smelc Apr 15, 2024

Choose a reason for hiding this comment

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

@Jimbo4350> this is what you had in mind right? Treating two cases in the same manner?

Copy link
Contributor

@Jimbo4350 Jimbo4350 Apr 15, 2024

Choose a reason for hiding this comment

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

You're almost there 👍

So we have:

acceptTxCDDLSerialisation
  :: FileOrPipe
  -> FileError TextEnvelopeError
  -> IO (Either CddlError CddlTx)
acceptTxCDDLSerialisation file err =
  case err of
   e@(FileError _ (TextEnvelopeDecodeError _)) ->
      first (CddlErrorTextEnv e) <$> readCddlTx file
   e@(FileError _ (TextEnvelopeAesonDecodeError _)) ->
      first (CddlErrorTextEnv e) <$> readCddlTx file
   e@(FileError _ (TextEnvelopeTypeError _ _)) ->
      first (CddlErrorTextEnv e) <$> readCddlTx file
   e@FileErrorTempFile{} -> return . Left $ CddlIOError e
   e@FileDoesNotExistError{} -> return . Left $ CddlIOError e
   e@FileIOError{} -> return . Left $ CddlIOError e

We cased on the TextEnvelope related errors because previously by default we used the deprecated "intermediate cli" format. Because it's no longer possible to create that format, and we will no longer support it, we can remove acceptTxCDDLSerialisation entirely and simply rely on the TextEnvelope encoding because we now default to the ledger's CDDL format.

Copy link
Contributor

@Jimbo4350 Jimbo4350 Apr 15, 2024

Choose a reason for hiding this comment

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

See below:

instance IsShelleyBasedEra era => SerialiseAsCBOR (Tx era) where
    serialiseToCBOR (ShelleyTx sbe tx) =
      shelleyBasedEraConstraints sbe $ serialiseShelleyBasedTx tx

    deserialiseFromCBOR _ bs =
      shelleyBasedEraConstraints (shelleyBasedEra :: ShelleyBasedEra era)
        $ deserialiseShelleyBasedTx (ShelleyTx shelleyBasedEra) bs

-- | The serialisation format for the different Shelley-based eras are not the
-- same, but they can be handled generally with one overloaded implementation.
--
serialiseShelleyBasedTx :: forall ledgerera .
                           L.EraTx ledgerera
                        => L.Tx ledgerera
                        -> ByteString
serialiseShelleyBasedTx = Plain.serialize'

We call ledger's serialize' indirectly in our SerialiseAsCBOR (Tx era) instance and TextEnvelope is serialized as follows:

class SerialiseAsCBOR a => HasTextEnvelope a where
    textEnvelopeType :: AsType a -> TextEnvelopeType

    textEnvelopeDefaultDescr :: a -> TextEnvelopeDescr
    textEnvelopeDefaultDescr _ = ""

serialiseToTextEnvelope :: forall a. HasTextEnvelope a
                        => Maybe TextEnvelopeDescr -> a -> TextEnvelope
serialiseToTextEnvelope mbDescr a =
    TextEnvelope {
      teType    = textEnvelopeType ttoken
    , teDescription   = fromMaybe (textEnvelopeDefaultDescr a) mbDescr
    , teRawCBOR = serialiseToCBOR a
    }
  where
    ttoken :: AsType a
    ttoken = proxyToAsType Proxy

Note the SerialiseAsCBOR constraint and the call to serialiseToCBOR in serialiseToTextEnvelope. Hope this makes things clearer!

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Looking good, see my comment below which hopefully clarifies things.

Comment on lines 534 to 535
return $ Right $ IncompleteCddlTxBody $ inAnyShelleyBasedEra sbe $ getTxBody tx
Right txBody -> return $ Right $ IncompleteCddlTxBody txBody
Copy link
Contributor

@Jimbo4350 Jimbo4350 Apr 15, 2024

Choose a reason for hiding this comment

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

You're almost there 👍

So we have:

acceptTxCDDLSerialisation
  :: FileOrPipe
  -> FileError TextEnvelopeError
  -> IO (Either CddlError CddlTx)
acceptTxCDDLSerialisation file err =
  case err of
   e@(FileError _ (TextEnvelopeDecodeError _)) ->
      first (CddlErrorTextEnv e) <$> readCddlTx file
   e@(FileError _ (TextEnvelopeAesonDecodeError _)) ->
      first (CddlErrorTextEnv e) <$> readCddlTx file
   e@(FileError _ (TextEnvelopeTypeError _ _)) ->
      first (CddlErrorTextEnv e) <$> readCddlTx file
   e@FileErrorTempFile{} -> return . Left $ CddlIOError e
   e@FileDoesNotExistError{} -> return . Left $ CddlIOError e
   e@FileIOError{} -> return . Left $ CddlIOError e

We cased on the TextEnvelope related errors because previously by default we used the deprecated "intermediate cli" format. Because it's no longer possible to create that format, and we will no longer support it, we can remove acceptTxCDDLSerialisation entirely and simply rely on the TextEnvelope encoding because we now default to the ledger's CDDL format.

Comment on lines 534 to 535
return $ Right $ IncompleteCddlTxBody $ inAnyShelleyBasedEra sbe $ getTxBody tx
Right txBody -> return $ Right $ IncompleteCddlTxBody txBody
Copy link
Contributor

@Jimbo4350 Jimbo4350 Apr 15, 2024

Choose a reason for hiding this comment

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

See below:

instance IsShelleyBasedEra era => SerialiseAsCBOR (Tx era) where
    serialiseToCBOR (ShelleyTx sbe tx) =
      shelleyBasedEraConstraints sbe $ serialiseShelleyBasedTx tx

    deserialiseFromCBOR _ bs =
      shelleyBasedEraConstraints (shelleyBasedEra :: ShelleyBasedEra era)
        $ deserialiseShelleyBasedTx (ShelleyTx shelleyBasedEra) bs

-- | The serialisation format for the different Shelley-based eras are not the
-- same, but they can be handled generally with one overloaded implementation.
--
serialiseShelleyBasedTx :: forall ledgerera .
                           L.EraTx ledgerera
                        => L.Tx ledgerera
                        -> ByteString
serialiseShelleyBasedTx = Plain.serialize'

We call ledger's serialize' indirectly in our SerialiseAsCBOR (Tx era) instance and TextEnvelope is serialized as follows:

class SerialiseAsCBOR a => HasTextEnvelope a where
    textEnvelopeType :: AsType a -> TextEnvelopeType

    textEnvelopeDefaultDescr :: a -> TextEnvelopeDescr
    textEnvelopeDefaultDescr _ = ""

serialiseToTextEnvelope :: forall a. HasTextEnvelope a
                        => Maybe TextEnvelopeDescr -> a -> TextEnvelope
serialiseToTextEnvelope mbDescr a =
    TextEnvelope {
      teType    = textEnvelopeType ttoken
    , teDescription   = fromMaybe (textEnvelopeDefaultDescr a) mbDescr
    , teRawCBOR = serialiseToCBOR a
    }
  where
    ttoken :: AsType a
    ttoken = proxyToAsType Proxy

Note the SerialiseAsCBOR constraint and the call to serialiseToCBOR in serialiseToTextEnvelope. Hope this makes things clearer!

@smelc smelc force-pushed the smelc/remove-UnwitnessedCliFormattedTxBody-constructor-v2 branch from eb15e23 to af3075a Compare May 16, 2024 10:03
@smelc smelc force-pushed the smelc/remove-UnwitnessedCliFormattedTxBody-constructor-v2 branch 3 times, most recently from 7190b09 to 4f4c790 Compare May 16, 2024 12:55
@smelc smelc marked this pull request as ready for review May 16, 2024 13:12
@smelc
Copy link
Contributor Author

smelc commented May 16, 2024

This one conflicts with #758 and I want this one to go first.

pure $ InAnyShelleyBasedEra era (getTxBody tx)
-- Why are we differentiating between a transaction body and a transaction?
InAnyShelleyBasedEra era txbody <- pure $ unIncompleteCddlTxBody unwitnessed
-- Why are we differentiating between a transaction body and a. newExceptT transaction?
Copy link
Contributor

@Jimbo4350 Jimbo4350 May 16, 2024

Choose a reason for hiding this comment

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

"newExceptT" creeped into the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, corrected 👍 and enqueueing for merge

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Nice work 💪. We can get this merged but what we eventually want to do is remove all references to "CDDL". We default to the ledger's serialization so we don't need to be explicit about this. Previously we did because the "intermediate cli tx body" format was a thing.

@smelc smelc force-pushed the smelc/remove-UnwitnessedCliFormattedTxBody-constructor-v2 branch from 4f4c790 to 1691ef3 Compare May 16, 2024 13:54
@smelc smelc enabled auto-merge May 16, 2024 13:55
@smelc smelc added this pull request to the merge queue May 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 16, 2024
@smelc smelc merged commit dd060e8 into main May 17, 2024
21 checks passed
@smelc smelc deleted the smelc/remove-UnwitnessedCliFormattedTxBody-constructor-v2 branch May 17, 2024 06:55
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.

Remove UnwitnessedCliFormattedTxBody constructor
2 participants