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.

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

Split compatible transaction building and signing #750

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

Split compatible transaction building and signing #750

wants to merge 1 commit into from
+75 −44

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Feb 11, 2025 •
edited
Loading

Changelog

- description: |
    Split compatible transaction building into separate building and signing functions
# 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

createCompatibleSignedTx wasn't really signing the transaction but just blindly adding signatures. It's split into two separate functions for building and signing.

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 Feb 11, 2025
@carbolymer carbolymer force-pushed the mgalazyn/fix/compatible-tx-build branch from ad9e94c to 04db64e Compare February 11, 2025 19:55
@carbolymer carbolymer marked this pull request as ready for review February 11, 2025 19:57
@carbolymer carbolymer mentioned this pull request Feb 12, 2025
Open
3 tasks
@carbolymer carbolymer force-pushed the mgalazyn/fix/compatible-tx-build branch from 04db64e to efef8d5 Compare February 13, 2025 20:17
@carbolymer carbolymer marked this pull request as draft February 13, 2025 20:17
@carbolymer carbolymer force-pushed the mgalazyn/fix/compatible-tx-build branch 2 times, most recently from 0bf7a3e to 4bdc261 Compare February 17, 2025 20:38
@carbolymer carbolymer force-pushed the mgalazyn/fix/compatible-tx-build branch from 4bdc261 to d11b7af Compare February 17, 2025 20:47
carbolymer
carbolymer commented Feb 17, 2025
View reviewed changes
cardano-api/src/Cardano/Api/Internal/Tx/Sign.hs
makeShelleyKeyWitness sbe (ShelleyTxBody _ txBody _ _ _ _) =
makeShelleyKeyWitness' sbe (A.TxBody txBody)

makeShelleyKeyWitness'
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 not sure what's the best approach here to naming and the argument for the version of this function working on ledger's tx body, so I went with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest changing the directory name space from: Cardano.Api.Internal.Tx.Compatible to Cardano.Api.Internal.Compatible.Tx. Then you can create Cardano.Api.Internal.Compatible.Witness and add the witness related functions there.

@carbolymer carbolymer requested a review from Jimbo4350 February 17, 2025 20:49
@carbolymer carbolymer marked this pull request as ready for review February 17, 2025 20:50
Jimbo4350
Jimbo4350 approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍. One suggestion.

cardano-api/src/Cardano/Api/Internal/Tx/Compatible.hs
@@ -164,11 +165,11 @@ createCompatibleSignedTx sbe ins outs witnesses txFee' anyProtocolUpdate anyVote
:: [(ScriptWitnessIndex, Certificate era, StakeCredential, Witness WitCtxStake era)]
indexedTxCerts = indexTxCertificates txCertificates'

allWitnesses
setScriptWitnesses
Copy link
Contributor

Choose a reason for hiding this comment

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

This just occurred to me. Does QA need backwards compatibility for script witnesses? If not we can drop it.

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 so. This was requested afair: https://github.com/IntersectMBO/cardano-cli/pull/972/files#diff-caae695482c364ec0098429ee19afc0089a97fe85de02cf1f81d3b029ef18109R41

cardano-api/src/Cardano/Api/Internal/Tx/Compatible.hs
@@ -224,3 +210,31 @@ createCommonTxBody era ins outs txFee' =
.~ Seq.fromList txOuts'
& L.feeTxBodyL
.~ txFee'

-- | Add provided witnesses to the transaction
addWitnesses
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest changing the directory name space from: Cardano.Api.Internal.Tx.Compatible to Cardano.Api.Internal.Compatible.Tx. Then you can add Cardano.Api.Internal.Compatible.Witness.

cardano-api/src/Cardano/Api/Internal/Tx/Sign.hs
makeShelleyKeyWitness sbe (ShelleyTxBody _ txBody _ _ _ _) =
makeShelleyKeyWitness' sbe (A.TxBody txBody)

makeShelleyKeyWitness'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest changing the directory name space from: Cardano.Api.Internal.Tx.Compatible to Cardano.Api.Internal.Compatible.Tx. Then you can create Cardano.Api.Internal.Compatible.Witness and add the witness related functions there.

cardano-api/src/Cardano/Api/Ledger/Lens.hs
@@ -83,6 +84,12 @@ strictMaybeL = lens g s
s :: StrictMaybe a -> Maybe a -> StrictMaybe a
s _ = maybe SNothing SJust

txToTxBodyL :: ShelleyBasedEra era -> Lens' (L.Tx (ShelleyLedgerEra era)) (TxBody era)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this called in the cli? If it's not lets not expose this because it encourages the use of TxBody.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used here: https://github.com/IntersectMBO/cardano-cli/pull/1057/files#diff-725abf998f7812014ea390b5cde9b1fe273d7af84e585bc47f37f9052399e7f9R286

Note that it's a TxBody from line 74, which is just a newtype over L.TxBody (ShelleyLedgerEra era)

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

@Jimbo4350 Jimbo4350 Jimbo4350 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

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

@palas palas Awaiting requested review from palas palas 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.
2 participants
@carbolymer @Jimbo4350