Skip to content

Conversation

@carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Jul 2, 2025

Changelog

- description: |
    cardano-rpc: Add cardano-rpc package with a dummy gRPC service
# 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

This PR adds new cardano-rpc cabal module.

It is not buildable with GHC 9.12 yet, so in GHA GHC 9.10 is used instead.

Tested in: IntersectMBO/cardano-node#6205

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 force-pushed the mgalazyn/feature/add-cardano-rpc branch 2 times, most recently from 825e7a0 to 6c7069d Compare July 2, 2025 15:53
@carbolymer carbolymer marked this pull request as ready for review July 2, 2025 16:10
@carbolymer carbolymer force-pushed the mgalazyn/feature/add-cardano-rpc branch from 6c7069d to 0d02419 Compare July 2, 2025 16:10
unicode: never
respectful: false
fixities: []
fixities:
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 missing fixity information was causing weird formatting issues with lenses in your PRs, @palas . I think this should help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nice!

@carbolymer carbolymer force-pushed the mgalazyn/feature/add-cardano-rpc branch from 0d02419 to 44c2834 Compare July 2, 2025 16:46
@carbolymer carbolymer self-assigned this Jul 2, 2025
@carbolymer carbolymer force-pushed the mgalazyn/feature/add-cardano-rpc branch 7 times, most recently from 5ddf6a4 to f5e18c6 Compare July 3, 2025 08:29
Comment on lines 177 to 184
- name: Setup tmate session
if: ${{ failure() }}
uses: mxschmitt/action-tmate@v3
with:
limit-access-to-actor: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
- name: Setup tmate session
if: ${{ failure() }}
uses: mxschmitt/action-tmate@v3
with:
limit-access-to-actor: true
# - name: Setup tmate session
# if: ${{ failure() }}
# uses: mxschmitt/action-tmate@v3
# with:
# limit-access-to-actor: true

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, how were you using this?

Copy link
Contributor Author

@carbolymer carbolymer Jul 4, 2025

Choose a reason for hiding this comment

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

You uncomment this, and then the CI action waits infinitely printing a ssh command, which allows you to interactively log in and troubleshoot on the GHA runner.

https://github.com/IntersectMBO/cardano-api/actions/runs/16048917967/job/45286704820#step:18:48

@carbolymer carbolymer force-pushed the mgalazyn/feature/add-cardano-rpc branch 4 times, most recently from 388ba81 to 9a2fb9c Compare July 3, 2025 11:14
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 PR needs to be broken up into smaller pieces. I suggest:

  • Open a PR with the dummy example.
  • Open a PR per mini-protocol along with their protobuf files.

strategy:
fail-fast: false
matrix:
ghc: ["9.6", "9.12"]
Copy link
Contributor

Choose a reason for hiding this comment

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

You weren't able to get it to build with 9.12?

Copy link
Contributor Author

@carbolymer carbolymer Jul 4, 2025

Choose a reason for hiding this comment

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

No, unfortunately no. I suspect the situation will improve once there will be stackage snapshot with 9.12, which means more of haskell packages will work with 9.12. Do we need 9.12 at this moment?

Copy link
Contributor

@Jimbo4350 Jimbo4350 Jul 4, 2025

Choose a reason for hiding this comment

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

Nop we don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @erikd


- name: "[Windows] Install grpc dependencies"
if: runner.os == 'Windows'
run: /usr/bin/pacman --noconfirm -S mingw-w64-x86_64-snappy mingw-w64-x86_64-protobuf
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines 177 to 184
- name: Setup tmate session
if: ${{ failure() }}
uses: mxschmitt/action-tmate@v3
with:
limit-access-to-actor: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, how were you using this?

// Represents a set of witnesses that validate a transaction
message WitnessSet {
repeated VKeyWitness vkeywitness = 1; // List of VKey witnesses.
repeated Script script = 2; // List of scripts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Redeemers and bootstrap witnesses should also be here.


// Represents a transaction in the Cardano blockchain.
message Tx {
repeated TxInput inputs = 1; // List of transaction inputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment with the currently outstanding fields?

Copy link
Contributor Author

@carbolymer carbolymer Jul 4, 2025

Choose a reason for hiding this comment

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

I wouldn't want to do that. I can mark them in haskell code, but I would like to keep the proto files identical to source files in UTxO RPC spec.

NewConstitutionAction new_constitution_action = 6; // Replace the Constitution

// TODO: revisit if there's a better way to handle this option because it doesn't actually need a value but proto syntax needs to require it
uint32 info_action = 7; // Info action should just be the integer number 6
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not:

diff --git a/cardano-rpc/proto/utxorpc/v1alpha/cardano/cardano.proto b/cardano-rpc/proto/utxorpc/v1alpha/cardano/cardano.proto
index e15f60eff..c96103366 100644
--- a/cardano-rpc/proto/utxorpc/v1alpha/cardano/cardano.proto
+++ b/cardano-rpc/proto/utxorpc/v1alpha/cardano/cardano.proto
@@ -129,7 +129,7 @@ message GovernanceAction {
     NewConstitutionAction new_constitution_action = 6; // Replace the Constitution
 
     // TODO: revisit if there's a better way to handle this option because it doesn't actually need a value but proto syntax needs to require it
-    uint32 info_action = 7; // Info action should just be the integer number 6
+    InfoAction info_action = 7; // Info action should just be the integer number 6
   }
 }
 
@@ -163,6 +163,8 @@ message NoConfidenceAction {
   GovernanceActionId gov_action_id = 1;
 }
 
+message InfoAction {}
+
 message UpdateCommitteeAction {
   GovernanceActionId gov_action_id = 1;
   repeated StakeCredential remove_committee_credentials = 2; // Committee members to remove (if any)

}

message TreasuryWithdrawalsAction {
repeated WithdrawalAmount withdrawals = 1; // A map of the withdrawals to make
Copy link
Contributor

Choose a reason for hiding this comment

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

This would make a list

// Represents a native script in Cardano.
message NativeScript {
oneof native_script {
bytes script_pubkey = 1; // Script based on an address key hash.
Copy link
Contributor

Choose a reason for hiding this comment

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

script_pubkey_hash?

@carbolymer carbolymer force-pushed the mgalazyn/feature/add-cardano-rpc branch 5 times, most recently from d1c9c89 to 99bd26b Compare July 8, 2025 11:47
@carbolymer carbolymer force-pushed the mgalazyn/feature/add-cardano-rpc branch from 4b5d1b8 to 9ad6d49 Compare July 8, 2025 12:21
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.

LGTM 👍

} = do
let isEnabled = fromMaybe False mIsEnabled
-- default to a some non-existing path. Does not matter if the gRPC endpoint is disabled
nodeSocketPath = fromMaybe "./node.socket" mNodeSocketPath
Copy link
Contributor

Choose a reason for hiding this comment

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

Abstracting this as a separate value (as we do for the node configuration) would be clearer IMO.

@carbolymer carbolymer force-pushed the mgalazyn/feature/add-cardano-rpc branch 4 times, most recently from 37851af to e77348c Compare July 8, 2025 14:29
@carbolymer carbolymer force-pushed the mgalazyn/feature/add-cardano-rpc branch from e77348c to e5a011d Compare July 8, 2025 14:33
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.

4 participants