Sort transaction fields in CBOR representation #785

carbolymer · 2025-03-19T18:40:03Z

Changelog

- description: |
    Sort transaction fields in CBOR representation
# 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

Implements:

[FR] - Flag for hardware wallet compatibility when building transactions cardano-cli#1086

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

cardano-api/src/Cardano/Api/Internal/Serialise/Cbor/Canonical.hs

…ialise.Cbor

cardano-api/src/Cardano/Api/Internal/Serialise/Cbor/Canonical.hs

Jimbo4350

LGTM, one comment about naming. I can approve after that's addressed.

Jimbo4350 · 2025-03-27T19:23:21Z

cardano-api/src/Cardano/Api/Internal/Serialise/Cbor/Canonical.hs

+import Data.List (sortBy)
+import Data.Tuple.Extra (both)
+
+-- | This function implements CBOR canonicalisation:


I would be clear we side step the other requirements of cip-21 by "canonicalizing" the bytes directly instead of "canonicalizing" a transaction body Haskell value. For instance we don't exclude proposal procedures and updates as stipulated in cip-21: https://github.com/cardano-foundation/CIPs/tree/master/CIP-0021#unsupported-entries.

Added clarification.

Jimbo4350 · 2025-03-27T19:25:53Z

cardano-api/src/Cardano/Api/Internal/Serialise/Cbor/Canonical.hs

+  -> Either DecoderError Term
+decodeTermFromBs input = do
+  (leftover, result) <-
+    first (DecoderErrorDeserialiseFailure "Cannot decode Term") $


Term has a Show instance. May be worthwhile including it in the error message.

But you don't have Term at this stage. I think DecoderDeserialiseFailure stores enough information to troubleshoot this:

DecoderErrorDeserialiseFailure :: Text -> CBOR.Read.DeserialiseFailure -> DecoderError DeserialiseFailure :: ByteOffset -> String -> DeserialiseFailure

I can add the input argument which didn't get deserialised to the error message, in hex/base64 I guess.

Jimbo4350 · 2025-03-28T18:09:06Z

cardano-api/src/Cardano/Api/Internal/SerialiseLedgerCddl.hs

+  -> File content Out
+  -> Tx era
+  -> IO (Either (FileError ()) ())
+writeTxFileTextEnvelopeCanonicalCddl sbe = writeTxFileTextEnvelopeCddl' sbe True


Not in this PR but we need to remove all mention of CDDL from cardano-api. I only introduced it because we had a non-CDDL transaction body format but that has been removed.

I've created: #795
Feel free to add more clarifications there.

Jimbo4350 · 2025-03-28T18:25:30Z

cardano-api/src/Cardano/Api/Internal/SerialiseLedgerCddl.hs

  :: ()
  => ShelleyBasedEra era
+  -> Bool


This should be a separate function. writeTxFileTextEnvelopeCanonical or something like that as it's specific to the HW wallet. I don't see the need to parameterize this on a Bool and call it in our non-canonicalized functions. If you insist on parameterizing it in this fashion then use a sum type instead of Bool.

We should also point out with a comment that it's not fully cip-21 compliant. For example we don't support: https://github.com/cardano-foundation/CIPs/tree/master/CIP-0021#unsupported-entries and we shouldn't IMO.

Refactored the functions. Added more information in haddock.

Jimbo4350

Nice work 👍

Jimbo4350 · 2025-04-03T19:00:32Z

cardano-api/src/Cardano/Api/Internal/SerialiseLedgerCddl.hs

+  canonicaliseTextEnvelopeCbor te = do
+    let canonicalisedTxBs =
+          either
+            (\err -> error $ "Impossible - deserialisation of just serialised bytes failed " <> show err)


Although it's impossible I would prepend writeTxFileTextEnvelopeCanonicalCddl to the error message.

Jimbo4350 · 2025-04-03T19:01:28Z

cardano-api/src/Cardano/Api/Internal/SerialiseLedgerCddl.hs

-  serialiseTxLedgerCddl era' tx' =
-    shelleyBasedEraConstraints era' $
-      serialiseToTextEnvelope (Just (TextEnvelopeDescr "Ledger Cddl Format")) tx'
+serialiseTxToTextEnvelope :: ShelleyBasedEra era -> Tx era -> TextEnvelope


carbolymer self-assigned this Mar 19, 2025

carbolymer force-pushed the mgalazyn/feature/sort-tx-maps branch from 3ec5bc6 to bbd8b1a Compare March 20, 2025 18:08

github-advanced-security bot found potential problems Mar 20, 2025

View reviewed changes

cardano-api/src/Cardano/Api/Internal/Serialise/Cbor/Canonical.hs Fixed Show fixed Hide fixed

Rename Cardano.Api.Internal.SerialiseCBOR -> Cardano.Api.Internal.Ser…

4bef964

…ialise.Cbor

carbolymer force-pushed the mgalazyn/feature/sort-tx-maps branch from d4a321c to e773a09 Compare March 21, 2025 17:58

carbolymer mentioned this pull request Mar 21, 2025

Add canonical CBOR toggle for transaction building and signing commands IntersectMBO/cardano-cli#1092

Open

3 tasks

carbolymer force-pushed the mgalazyn/feature/sort-tx-maps branch 3 times, most recently from 2b767ad to e788250 Compare March 27, 2025 10:40

Implement CBOR canonicalisation according to CIP-21

80d439d

carbolymer force-pushed the mgalazyn/feature/sort-tx-maps branch from e788250 to 80d439d Compare March 27, 2025 14:21

carbolymer marked this pull request as ready for review March 27, 2025 14:25

carbolymer requested review from dcoutts, Jimbo4350, erikd, newhoggy, smelc, palas, kevinhammond, CarlosLopezDeLara and disassembler as code owners March 27, 2025 14:25

carbolymer commented Mar 27, 2025

View reviewed changes

cardano-api/src/Cardano/Api/Internal/Serialise/Cbor/Canonical.hs Outdated Show resolved Hide resolved

Jimbo4350 reviewed Mar 28, 2025

View reviewed changes

carbolymer force-pushed the mgalazyn/feature/sort-tx-maps branch from a4fd0cb to e840bd6 Compare March 31, 2025 15:11

Review remarks

43ecd64

carbolymer force-pushed the mgalazyn/feature/sort-tx-maps branch from e840bd6 to 43ecd64 Compare April 1, 2025 13:28

Jimbo4350 approved these changes Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort transaction fields in CBOR representation #785

Sort transaction fields in CBOR representation #785

carbolymer commented Mar 19, 2025 •

edited

Loading

Jimbo4350 left a comment

Jimbo4350 Mar 27, 2025

carbolymer Mar 31, 2025

Jimbo4350 Mar 27, 2025

carbolymer Mar 31, 2025

Jimbo4350 Mar 28, 2025

carbolymer Mar 31, 2025

Jimbo4350 Mar 28, 2025 •

edited

Loading

carbolymer Mar 31, 2025

Jimbo4350 left a comment

Jimbo4350 Apr 3, 2025

Jimbo4350 Apr 3, 2025

Sort transaction fields in CBOR representation #785

Are you sure you want to change the base?

Sort transaction fields in CBOR representation #785

Conversation

carbolymer commented Mar 19, 2025 • edited Loading

Changelog

Context

Checklist

Jimbo4350 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbolymer commented Mar 19, 2025 •

edited

Loading

Jimbo4350 Mar 28, 2025 •

edited

Loading