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

Byron isolation 2 of n #467

Merged
merged 10 commits into from
Nov 22, 2023
Merged

Byron isolation 2 of n #467

merged 10 commits into from
Nov 22, 2023

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Nov 20, 2023

Changelog

- description: |
    Further separation of Byron from the Shelley based eras
# 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
  - improvement    # QoL changes e.g. refactoring
  # - 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

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

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

@Jimbo4350 Jimbo4350 force-pushed the jordan/byron-isolation-2-of-n-final branch 2 times, most recently from 4421bcf to 67e9223 Compare November 20, 2023 15:32
-- TODO: As a follow up we need to expose a simple tx building command that only
-- uses inputs, outputs and update proposals. NB: Update proposals are a separate
-- thing in the Byron era so we need to figure out how we are handling that at the
-- cli command level.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a follow up PR we will remove support for the Byron era here.

@@ -930,8 +930,7 @@ runTransactionSignCmd
inputTxFile <- liftIO $ fileOrPipe inputTxFilePath
anyTx <- lift (readFileTx inputTxFile) & onLeft (left . TxCmdCddlError)

InAnyShelleyBasedEra sbe tx <-
onlyInShelleyBasedEras "sign for Byron era transactions" anyTx
InAnyShelleyBasedEra sbe tx <- pure anyTx
Copy link
Contributor Author

@Jimbo4350 Jimbo4350 Nov 20, 2023

Choose a reason for hiding this comment

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

We never supported Byron era tx signing. Indication that people were using the "old" cardano-cli byron cli commands for everything Byron related.

@@ -998,8 +997,8 @@ runTransactionSubmitCmd
, txFile
} = do
txFileOrPipe <- liftIO $ fileOrPipe txFile
InAnyCardanoEra era tx <- lift (readFileTx txFileOrPipe) & onLeft (left . TxCmdCddlError)
let txInMode = TxInMode era tx
InAnyShelleyBasedEra era tx <- lift (readFileTx txFileOrPipe) & onLeft (left . TxCmdCddlError)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

QA confirmed they use the "old" cardano-cli byron commands to submit txs. Unlikely anybody else is using this command to submit byron era txs.

@Jimbo4350 Jimbo4350 force-pushed the jordan/byron-isolation-2-of-n-final branch from 67e9223 to c907b16 Compare November 20, 2023 16:18
@Jimbo4350 Jimbo4350 marked this pull request as ready for review November 20, 2023 16:19
@Jimbo4350 Jimbo4350 force-pushed the jordan/byron-isolation-2-of-n-final branch 2 times, most recently from a020a6b to d704050 Compare November 20, 2023 17:11
@Jimbo4350 Jimbo4350 force-pushed the jordan/byron-isolation-2-of-n-final branch from d704050 to ebdfd13 Compare November 20, 2023 19:45
@Jimbo4350 Jimbo4350 requested review from a team as code owners November 22, 2023 11:38
Introduce Byron/Shelley split into runLegacyTransactionBuildRawCmd
The following functions never supported the Byron era:
runTransactionSignCmd
runTransactionCalculateMinFeeCmd
runTransactionWitnessCmd
cardano-cli transaction submit
cardano-cli transaction txid
cardano-cli transaction view
Rename readFileInAnyCardanoEra to readFileInAnyShelleyBasedEra
Modify CddlWitness to wrap InAnyShelleyBasedEra KeyWitness
@Jimbo4350 Jimbo4350 force-pushed the jordan/byron-isolation-2-of-n-final branch 2 times, most recently from 026262a to cebac91 Compare November 22, 2023 12:05
@Jimbo4350 Jimbo4350 force-pushed the jordan/byron-isolation-2-of-n-final branch from cebac91 to ffd8f5c Compare November 22, 2023 14:52
@Jimbo4350 Jimbo4350 enabled auto-merge November 22, 2023 14:53
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Nov 22, 2023
Merged via the queue into main with commit 4c67eb5 Nov 22, 2023
19 checks passed
@Jimbo4350 Jimbo4350 deleted the jordan/byron-isolation-2-of-n-final branch November 22, 2023 15:31
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.

2 participants