Skip to content

cardano-rpc | Add decoded PlutusData and NativeScript in proto definition #947

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 4 commits into
base: master
Choose a base branch
from
Open

cardano-rpc | Add decoded PlutusData and NativeScript in proto definition #947

wants to merge 4 commits into from

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Sep 5, 2025 •
edited by palas
Loading

Changelog

- description: |
    cardano-rpc | Add decoded PlutusData and NativeScript in proto definition #947 
# 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...
# uncomment at least one main project this PR is associated with
  projects:
  # - cardano-api
  # - cardano-api-gen
  # - cardano-rpc
  # - cardano-wasm

Context

Previously, NativeScript and PlutusData were omitted in the ReadUtxos query feature:

This PR adds decoding of those values to the protobuf type. Using protobuf-defined values instead of CBOR blobs is advantageous, as it lets UTxO RPC consumers work with a single data format.

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

@carbolymer carbolymer self-assigned this Sep 5, 2025
@carbolymer carbolymer changed the title cardano-rpc | Add decoded PlutusData to Datum in proto definition cardano-rpc | Add decoded PlutusData and NativeScript in proto definition Sep 8, 2025
@carbolymer carbolymer force-pushed the mgalazyn/feature/rpc-add-plutusdata-support branch from 2a68c4a to 0da56f8 Compare September 8, 2025 14:15
@carbolymer carbolymer marked this pull request as ready for review September 8, 2025 14:45
palas
palas approved these changes Sep 8, 2025
View reviewed changes
Copy link
Contributor

@palas palas left a comment

Choose a reason for hiding this comment

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

I think it is all accurate except for the bigint representation, which I am sure you already considered. But I commented on the bits that I found suspicious, even though I think they are all correct

cardano-rpc/src/Cardano/Rpc/Server/Internal/Orphans.hs Outdated
ScriptDataBytes bs ->
defMessage & #boundedBytes .~ bs
ScriptDataNumber int ->
defMessage & #bigInt . #int .~ fromIntegral int
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing this is limited to 64bit, but provisionally should work

Copy link
Contributor Author

@carbolymer carbolymer Sep 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, actually good catch. I'm converting unbounded integer to uint64 here. I think we should safeguard ourselves from partial conversion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a fix for that.

cardano-rpc/src/Cardano/Rpc/Server/Internal/Orphans.hs
Comment on lines +126 to +129
let constr =
defMessage
& #tag .~ fromIntegral tag
& #fields .~ map inject args
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this is missing any_constructor, don't know whether that is important

Jimbo4350
Jimbo4350 reviewed Sep 8, 2025
View reviewed changes
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.

This LGTM however you should start implementing round trip tests for your Inject instances and consider having a separate type class for these transformations.

I'll approve once the round trip tests are added.

cardano-rpc/src/Cardano/Rpc/Server/Internal/Orphans.hs
PlutusScript PlutusScriptV3 ps ->
defMessage & #plutusV3 .~ serialiseToRawBytes ps

instance Inject SimpleScript (Proto UtxoRpc.NativeScript) where
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on having separate (more rpc specific) type class for this instead of using Inject?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll create specific conversion functions as per https://github.com/input-output-hk/cardano-node-wiki/wiki/ADR-014-Total-conversion-functions-conventions#explicit-conversion-functions

A type class feels a bit shoehorned here. Some conversions will be partial.

@carbolymer carbolymer force-pushed the mgalazyn/feature/rpc-move-address-array-to-cardano-proto branch from f857964 to a345801 Compare September 9, 2025 11:39
Base automatically changed from mgalazyn/feature/rpc-move-address-array-to-cardano-proto to master September 10, 2025 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 left review comments

@palas palas palas approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts dcoutts 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

Assignees

@carbolymer carbolymer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carbolymer @palas @Jimbo4350