Skip to content

Add SerialiseAsCBOR instance for TxOut #960

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add SerialiseAsCBOR instance for TxOut #960

wants to merge 3 commits into from
+40 −2

Conversation

palas
Copy link
Contributor

@palas palas commented Sep 18, 2025 •
edited
Loading

Changelog

- description: |
    Added `HasTypeProxy` `FromCBOR`, `ToCBOR`, and `SerialiseAsCBOR ` instances to `TxOut`.
  type:
  - feature
  projects:
  - cardano-api

Context

This PR addresses #955.

How to trust this PR

The test should help with trust. But there is a bit of an issue with ToCBOR instance losing information, so there is a design decision to be make about this. Please, let me know what you think about this.

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

@palas palas self-assigned this Sep 18, 2025
@palas palas linked an issue Sep 18, 2025 that may be closed by this pull request
make a deserialiseFromCbor instance for TxOut #955
Open
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 18, 2025
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 22, 2025
View reviewed changes
@palas palas force-pushed the SerialiseAsCBOR-for-TxOut branch from e9c48c5 to 753f06b Compare September 22, 2025 17:19
carbolymer
carbolymer requested changes Sep 22, 2025
View reviewed changes
cardano-api/test/cardano-api-test/Test/Cardano/Api/CBOR.hs
Comment on lines +124 to +125
(TxOutSupplementalDatum aeo d) ->
TxOut aie val (TxOutDatumHash aeo (hashScriptDataBytes d)) rs
Copy link
Contributor

@carbolymer carbolymer Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure how usable will be such deserialised txout. I think after you convert datum to hash, it's no longer included in the transaction datums: https://github.com/IntersectMBO/cardano-api/pull/814/files?diff=split&w=1#diff-dbfef7b82f6c05596cde61360e58ef674ad0bea7ac8c3e416178ccbde84b98b9R3107

Tactical merge block, since we're still discussing design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer requested changes

@dcoutts dcoutts Awaiting requested review from dcoutts dcoutts is a code owner

@Jimbo4350 Jimbo4350 Awaiting requested review from Jimbo4350 Jimbo4350 is a code owner

@erikd erikd Awaiting requested review from erikd erikd is a code owner

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara CarlosLopezDeLara is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@palas palas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make a deserialiseFromCbor instance for TxOut
2 participants
@palas @carbolymer