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

Add new field 'inlineDatumRaw' to TxOut ToJSON instance #632

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

ffakenz
Copy link
Contributor

@ffakenz ffakenz commented Sep 11, 2024

Changelog

- description: |
    Add new field 'inlineDatumRaw' to TxOut ToJSON instance

    It contains the raw CBOR for any inline datum.

    When building applications that need to spend from a script UTxO which has a datum attached,
    that off-chain code needs access to the raw Datum for evaluating the transaction and calculate fees.

# 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

When building applications that need to spend from a script UTxO which has a datum attached,
that off-chain code needs access to the raw Datum for evaluating the transaction and calculate fees.

How to trust this PR

This item is a backward-compatible change.

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

@smelc
Copy link
Contributor

smelc commented Sep 11, 2024

Code LGTM but I'm more concerned about consistency. Other fields have JSON data, whereas this new one has CBOR. Isn't this new CBOR redundant with what's in the inlineDatum field?

@ffakenz
Copy link
Contributor Author

ffakenz commented Sep 11, 2024

Yes, you make a good point; it's a bit redundant, but the idea is that it helps users more conveniently compute fees.

See this ticket - cardano-scaling/hydra#1543

I understand if it doesn't make sense to include here; we can find another way to work around it; but ultimately we do want to return this type to improve the UX of our return values for our clients.

@ch1bo
Copy link
Contributor

ch1bo commented Sep 12, 2024

idea is that it helps users more conveniently compute fees.

Not only more conveniently, but otherwise impossible to compute as the exact binary representation is needed for evaluating scripts (or computing script integrity hashes).

@smelc
Copy link
Contributor

smelc commented Sep 12, 2024

@ffakenz> can you fix the formatting of the files you modified? You need to run fourmolu -i ...; stylish-haskell -i ... on the files you modified. Both fourmolu and stylish-haskell are provided in the Nix flake.

@ffakenz ffakenz force-pushed the txout-json-raw-cbor-datum branch 2 times, most recently from cb548d8 to c78432b Compare September 12, 2024 17:50
@smelc
Copy link
Contributor

smelc commented Sep 17, 2024

@ffakenz> you need to sign your commits:

image

and it's good to go

@ffakenz ffakenz force-pushed the txout-json-raw-cbor-datum branch 6 times, most recently from 814e406 to 7ad4e25 Compare September 23, 2024 21:33
@smelc
Copy link
Contributor

smelc commented Sep 24, 2024

@ffakenz> can you sign your commits again?

You can setup your git to do that automatically: https://stackoverflow.com/a/20628522

auto-merge was automatically disabled September 24, 2024 17:40

Head branch was pushed to by a user without write access

@ffakenz
Copy link
Contributor Author

ffakenz commented Sep 24, 2024

@ffakenz> can you sign your commits again?

You can setup your git to do that automatically: https://stackoverflow.com/a/20628522

Apologies for that.
This tends to happen whenever I click "Update with rebase."
I'll make sure to sign my commits again.
Sorry for the inconvenience.

It contains the raw CBOR for any inline datum.

Reason:
> When building applications that need to spend from a script UTxO which has a datum attached  (like in a Hydra head),
that off-chain code needs access to the raw Datum for evaluating the transaction and calculate fees.
@smelc smelc added this pull request to the merge queue Sep 25, 2024
Merged via the queue into IntersectMBO:main with commit 17eb46f Sep 25, 2024
25 checks passed
github-merge-queue bot pushed a commit to cardano-scaling/hydra that referenced this pull request Oct 9, 2024
Addresses #1543 and tests whether
IntersectMBO/cardano-api#632 results in `GET
/snapshot/utxo` to contain `inlineDatumRaw`.

Bonus: First step on consolidating `TxOut` and `UTxO` generators
(deduplicating and moving them to a common module)

TODO: Somehow get
IntersectMBO/cardano-api@17eb46f
into this branch. Most likely requiring #1680 (which updates to most
recent `cardano-api` release `9.30`) and a `source-repository-package`
onto some unreleased cardano-api.

---

* [x] CHANGELOG updated or not needed
* [x] Documentation updated or not needed
* [x] Haddocks updated or not needed
* [x] No new TODOs introduced or explained herafter
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.

5 participants