Skip to content

Conversation

@qnikst
Copy link
Collaborator

@qnikst qnikst commented Sep 25, 2025

Description

Checklist

  • Commits in meaningful sequence and with useful messages.
  • Tests added or updated when needed.
  • CHANGELOG.md files updated for packages with externally visible changes.
    NOTE: New section is never added with the code changes. (See RELEASING.md).
  • Versions updated in .cabal and CHANGELOG.md files when necessary, according to the
    versioning process.
  • Version bounds in .cabal files updated when necessary.
    NOTE: If bounds change in a cabal file, that package itself must have a version increase. (See RELEASING.md).
  • Code formatted (use scripts/fourmolize.sh).
  • Cabal files formatted (use scripts/cabal-format.sh).
  • CDDL files are up to date (use scripts/gen-cddl.sh)
  • hie.yaml updated (use scripts/gen-hie.sh).
  • Self-reviewed the diff.

@qnikst qnikst marked this pull request as draft September 25, 2025 18:39
LedgerCBOR instance — allows to generate CanonicalCBOR instance from the ledger one
MempackCBOR instance - allow to generate CanonicalCBOR instance from the mempack one
Copy link
Member

@nc6 nc6 left a comment

Choose a reason for hiding this comment

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

Sorry, somehow I never submitted my review for this...

import Cardano.Ledger.Alonzo.TxOut (DataHash32, Addr28Extra)

-- | Helper that allows us to deriving instances via internal CBOR representation
newtype LedgerCBOR a = LedgerCBOR { unLedgerCBOR :: a }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe integrate the version as a parameter to LedgerCBOR, then we can derive for specific versions rather than hard-coding to V1?

Copy link
Collaborator Author

@qnikst qnikst Sep 26, 2025

Choose a reason for hiding this comment

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

Let's try! Not sure how it would work with presence of the new versions, but it will likely to simplify current version

UPD: done

deriving (Eq, Show)

instance (MemPack a) => ToCanonicalCBOR V1 (MemPackCBOR a) where
toCanonicalCBOR _v (MemPackCBOR a) = toPlainEncoding shelleyProtVer (encodeMemPack a)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to go via the ledger encodings specifically for MemPack? I would have thought we would just encode the ByteArray directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This approach creates CBOR bytes, and stores MemPack there, so basically it stores to ByteArray in the encoder. It should be compatible with current implementation and reuse optimal approach from the ledger,

This approach should be compatible with CBOR for SCLS files and reuse all functionality from ledger.

However in this approach we will have to write specification for the binary form in CIP.

Maybe I miss something still.

fromCanonicalCBOR = Versioned . MemPackCBOR <$> toPlainDecoder Nothing shelleyProtVer decodeMemPack

-- | Input wrapper for the keys that are used in utxo namespace
data UtxoIn = UtxoIn { utxoInAddress :: DataHash32, utxoInIndex :: Word16 }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this 'UtxoKey' rather than 'UtxoIn', which is a little confusing. Or just re-use the TxIn datatype

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reusing TxIn would be nice. Though IIUC there is no mapping from TxIn to script data, so should I use two variants for TxIn and for Purpose and Hash (for scripts)?

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.

3 participants