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 certificates to CLI interface in compatible transaction-sign #972

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Nov 18, 2024

Changelog

- description: |
    Add certificates to CLI interface in `compatible transaction-sign` 
# 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

Requires:

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/fix/add-certs-to-compatible-transaction-sign branch 6 times, most recently from bd0fc1e to 26a1017 Compare November 26, 2024 16:45
@carbolymer carbolymer force-pushed the mgalazyn/fix/add-certs-to-compatible-transaction-sign branch from 26a1017 to dbc9255 Compare November 28, 2024 15:26
@carbolymer carbolymer force-pushed the mgalazyn/fix/add-certs-to-compatible-transaction-sign branch 4 times, most recently from 6473d28 to ac19e61 Compare January 3, 2025 16:41
@carbolymer carbolymer marked this pull request as ready for review January 3, 2025 16:48
@smelc smelc added the next-update Needs a dependency to be updated to be merged label Jan 6, 2025
(protocolUpdates, votes) :: (AnyProtocolUpdate era, AnyVote era) <-
caseShelleyToBabbageOrConwayEraOnwards
( const $ do
prop <- maybe (pure $ NoPParamsUpdate sbe) readUpdateProposalFile mUpdateProposal
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's not possible to move this one out of the case, to share the code between the two branches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really, those are two different files with different purposes

@smelc
Copy link
Contributor

smelc commented Jan 6, 2025

Code LGTM ✔️

cc @Jimbo4350 for approval

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! Two minor comments.


instance Error CompatibleTransactionError where
prettyError = \case
CompatibleTxOutError e -> renderTxCmdError e
CompatibleTxCmdError e -> renderTxCmdError e
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the name change? This error is TxOut related.

Copy link
Contributor Author

@carbolymer carbolymer Jan 15, 2025

Choose a reason for hiding this comment

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

I disagree, TxCmdError is much broader than just transaction outputs. I'm also reusing it in line 296.


runCompatibleTransactionCmd
:: CompatibleTransactionCmds era -> ExceptT CompatibleTransactionError IO ()
:: forall era
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary forall era

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'm using scopedtypevariables to have the same era in functions in where

Copy link
Contributor

@Jimbo4350 Jimbo4350 Jan 15, 2025

Choose a reason for hiding this comment

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

Are you using it for readability purposes? If not you can remove all mentions of era in the function body of runCompatibleTransactionCmd. It still builds:

diff --git a/cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs b/cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs
index 25468707e..606a0945d 100644
--- a/cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs
+++ b/cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs
@@ -2,7 +2,6 @@
 {-# LANGUAGE ExistentialQuantification #-}
 {-# LANGUAGE GADTs #-}
 {-# LANGUAGE LambdaCase #-}
-{-# LANGUAGE ScopedTypeVariables #-}
 {-# LANGUAGE TupleSections #-}
 
 module Cardano.CLI.Compatible.Transaction
@@ -218,8 +217,7 @@ instance Error CompatibleTransactionError where
     CompatibleScriptWitnessError e -> renderScriptWitnessError e
 
 runCompatibleTransactionCmd
-  :: forall era
-   . CompatibleTransactionCmds era
+  :: CompatibleTransactionCmds era
   -> ExceptT CompatibleTransactionError IO ()
 runCompatibleTransactionCmd
   ( CreateCompatibleSignedTransaction
@@ -243,7 +241,7 @@ runCompatibleTransactionCmd
       firstExceptT CompatibleScriptWitnessError $
         readScriptWitnessFiles sbe certificates
 
-    certsAndMaybeScriptWits :: [(Certificate era, Maybe (ScriptWitness WitCtxStake era))] <-
+    certsAndMaybeScriptWits  <-
       shelleyBasedEraConstraints sbe $
         sequence
           [ fmap
@@ -254,7 +252,7 @@ runCompatibleTransactionCmd
           | (CertificateFile certFile, mSwit) <- certFilesAndMaybeScriptWits
           ]
 
-    (protocolUpdates, votes) :: (AnyProtocolUpdate era, AnyVote era) <-
+    (protocolUpdates, votes) <-
       caseShelleyToBabbageOrConwayEraOnwards
         ( const $ do
             prop <- maybe (pure $ NoPParamsUpdate sbe) readUpdateProposalFile mUpdateProposal
@@ -326,26 +324,17 @@ runCompatibleTransactionCmd
       newExceptT $
         writeTxFileTextEnvelopeCddl sbe outputFp signedTx
    where
-    convertCertificates
-      :: [(Certificate era, Maybe (ScriptWitness WitCtxStake era))]
-      -> TxCertificates BuildTx era
     convertCertificates certsAndScriptWitnesses =
       TxCertificates sbe certs $ BuildTxWith reqWits
      where
       certs = map fst certsAndScriptWitnesses
       reqWits = fromList $ mapMaybe convert' certsAndScriptWitnesses
-      convert'
-        :: (Certificate era, Maybe (ScriptWitness WitCtxStake era))
-        -> Maybe (StakeCredential, Witness WitCtxStake era)
       convert' (cert, mScriptWitnessFiles) = do
         sCred <- selectStakeCredentialWitness cert
         Just . (sCred,) $ case mScriptWitnessFiles of
           Just sWit -> ScriptWitness ScriptWitnessForStakeAddr sWit
           Nothing -> KeyWitness KeyWitnessForStakeAddr
 
-    validateTxInsReference
-      :: [TxIn]
-      -> Either TxCmdError (TxInsReference era)
     validateTxInsReference [] = return TxInsReferenceNone
     validateTxInsReference allRefIns = do
       let era = toCardanoEra era

Copy link
Contributor Author

@carbolymer carbolymer Jan 16, 2025

Choose a reason for hiding this comment

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

Yes it's mostly for readability. If you prefer to not have type signatures, I can remove them.

@neilmayhew
Copy link
Contributor

I've cherry-picked the non-SRP commit to #986 where it's needed for the version of api that's about to be released. However, I see now that there are a few minor comments still needing to be addressed. Could you please resolve those and then I can re-pick the commit.

@carbolymer
Copy link
Contributor Author

carbolymer commented Jan 15, 2025

@neilmayhew thanks for letting me know. I'll merge this PR separately - I've disabled the feature for now in the integration branch.

@carbolymer carbolymer force-pushed the mgalazyn/fix/add-certs-to-compatible-transaction-sign branch 2 times, most recently from e3c153b to d554312 Compare January 15, 2025 14:30
@carbolymer carbolymer requested review from a team as code owners January 15, 2025 14:30
@carbolymer carbolymer force-pushed the mgalazyn/fix/add-certs-to-compatible-transaction-sign branch from d554312 to 3b889d3 Compare January 15, 2025 14:37
@carbolymer carbolymer force-pushed the mgalazyn/fix/add-certs-to-compatible-transaction-sign branch from 3b889d3 to f7346ef Compare January 15, 2025 14:45
@carbolymer carbolymer enabled auto-merge January 15, 2025 14:45
* Added reference test for `compatible conway transaction singed-transaction`
@carbolymer carbolymer force-pushed the mgalazyn/fix/add-certs-to-compatible-transaction-sign branch from f7346ef to 0521006 Compare January 16, 2025 13:51
@carbolymer carbolymer added this pull request to the merge queue Jan 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-update Needs a dependency to be updated to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants