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

Remove eras older than Babbage support in transaction build and transaction build-estimate #878

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Remove eras older than Babbage support in transaction build and transaction build-estimate #878

merged 1 commit into from
Sep 20, 2024

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Aug 30, 2024 •
edited
Loading

Changelog

- description: |
    Remove eras older than Babbage support in `transaction build` and `transaction build-estimate`
# 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 cardano-api with:

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/remove-older-eras-transaction-build branch 9 times, most recently from 9440560 to c5e1eda Compare September 4, 2024 13:26
@carbolymer carbolymer changed the title Remove older eras in transaction build Remove older eras support in transaction build Sep 4, 2024
@carbolymer carbolymer changed the title Remove older eras support in transaction build Remove eras older than Babbage support in transaction build Sep 4, 2024
@carbolymer carbolymer force-pushed the mgalazyn/feature/remove-older-eras-transaction-build branch 5 times, most recently from 460f8a0 to 7ea384c Compare September 6, 2024 06:47
@carbolymer carbolymer changed the title Remove eras older than Babbage support in transaction build Remove eras older than Babbage support in transaction build and transaction build-estimate Sep 6, 2024
@carbolymer carbolymer marked this pull request as ready for review September 6, 2024 06:48
@carbolymer carbolymer force-pushed the mgalazyn/feature/remove-older-eras-transaction-build branch from 7ea384c to 2166c72 Compare September 6, 2024 06:54
smelc
smelc approved these changes Sep 6, 2024
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Options/Transaction.hs Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs
@@ -386,7 +382,7 @@ runTransactionBuildEstimateCmd -- TODO change type
txOuts <- mapM (toTxOutInAnyEra sbe) txouts

-- the same collateral input can be used for several plutus scripts
let filteredTxinsc = toList @(Set _) $ fromList txInsCollateral
let filteredTxinsc = nubOrd txInsCollateral
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, I didn't knew about nubOrd 💯

@gitmachtl
Copy link
Contributor

would this affect the build-raw command in any way? it should still be possible to do all things with build-raw imo.

@carbolymer
Copy link
Contributor Author

@gitmachtl no it's only for build and build-estimate. You can see which commands are removed here: https://github.com/IntersectMBO/cardano-cli/pull/878/files?file-filters%5B%5D=.cli&show-deleted-files=true&show-viewed-files=true&w=1
build-raw stays the same as it was.

@carbolymer carbolymer force-pushed the mgalazyn/feature/remove-older-eras-transaction-build branch from 2166c72 to d243fc8 Compare September 9, 2024 15:18
Jimbo4350
Jimbo4350 reviewed Sep 9, 2024
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.

A few comments. This needs to be updated as mainnet is now in the Conway era.

cardano-cli/src/Cardano/CLI/EraBased/Options/Transaction.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Legacy/Options.hs
[ Opt.flag' (Exp.Some Exp.BabbageEra) $
mconcat
[ Opt.long "babbage-era"
, Opt.help "Specify the Babbage era (default)"
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be updated because mainnet is now in the Conway 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.

Does it have to be updated though? Changing this will also change the behaviour of legacy commands, like making some options mandatory, suddenly.

If we're going to ditch legacy and latest, I would see this as a slight inconvenience for users which would accelerate the switch to era-based commands hierarchy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I changed my mind while working on some deprecations in the cli. We are going to keep legacy and latest.

cardano-cli/src/Cardano/CLI/Legacy/Run/Transaction.hs
@@ -168,7 +169,7 @@ runLegacyTransactionCmds = \case
runLegacyTransactionBuildCmd
:: ()
=> SocketPath
-> EraInEon ShelleyBasedEra
-> Exp.Some Exp.Era
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to introduce a deprecation message for the legacy commands as well.

Copy link
Contributor Author

@carbolymer carbolymer Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've added deprecation for legacy transaction.

I think you've started working on this:

so it becomes out of scope of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR can be merged as is. I'll rebase mine on it.

cardano-cli/src/Cardano/CLI/Environment.hs

envCliAnyShelleyToBabbageEra :: EnvCli -> Maybe (EraInEon ShelleyToBabbageEra)
envCliAnyShelleyToBabbageEra envCli = do
envCliAnyEon :: Typeable eon => Eon eon => EnvCli -> Maybe (EraInEon eon)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is called in parsers like pShelleyBasedShelley which are not used. We can delete them.

Copy link
Contributor Author

@carbolymer carbolymer Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it's also used in pAnyShelleyToBabbageEra and in pAnyShelleyBasedEra which are still in use.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to parsers like pShelleyBasedShelley which I see have been removed already.

@gitmachtl
Copy link
Contributor

one question, so in the future if will not be possible to generate "shelley-era" style transactions anymore if you just wanna send lovelaces around, right?

@Jimbo4350
Copy link
Contributor

Jimbo4350 commented Sep 10, 2024 •
edited
Loading

one question, so in the future if will not be possible to generate "shelley-era" style transactions anymore if you just wanna send lovelaces around, right?

We will expose a transaction building command that will be parameterized on the era. Currently it will only be able to spend tx inputs and accept update proposals. This is for our QA team to test hardforks from Byron -> $currentEra.

@carbolymer carbolymer force-pushed the mgalazyn/feature/remove-older-eras-transaction-build branch from d243fc8 to 6b65485 Compare September 10, 2024 15:06
@carbolymer carbolymer force-pushed the mgalazyn/feature/remove-older-eras-transaction-build branch from 6b65485 to 803485a Compare September 20, 2024 11:37
@carbolymer carbolymer force-pushed the mgalazyn/feature/remove-older-eras-transaction-build branch from 803485a to 9ab866f Compare September 20, 2024 17:50
@carbolymer carbolymer force-pushed the mgalazyn/feature/remove-older-eras-transaction-build branch from 9ab866f to 20fa071 Compare September 20, 2024 17:58
@carbolymer carbolymer added this pull request to the merge queue Sep 20, 2024
Merged via the queue into main with commit a157c49 Sep 20, 2024
25 checks passed
@carbolymer carbolymer deleted the mgalazyn/feature/remove-older-eras-transaction-build branch September 20, 2024 18:49
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

@smelc smelc smelc approved these changes

@newhoggy newhoggy Awaiting requested review from newhoggy

@palas palas Awaiting requested review from palas

@kevinhammond kevinhammond Awaiting requested review from kevinhammond

@disassembler disassembler Awaiting requested review from disassembler

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@carbolymer @gitmachtl @Jimbo4350 @smelc