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

Initial Byron isolation 1 of n #450

Closed
wants to merge 7 commits into from
Closed

Initial Byron isolation 1 of n #450

wants to merge 7 commits into from

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Nov 10, 2023 •
edited
Loading

Changelog

- description: |
    Collateral inputs and the minimum value calculation are not available in the Byron era. Update the code to reflect this.
# 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-1-of-n branch 4 times, most recently from c0d772d to d78fa54 Compare November 14, 2023 17:02
Jimbo4350
Jimbo4350 commented Nov 14, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Run/Transaction.hs
-- TODO: shouldn't we just require shelley based era here instead of error-ing for byron?
forEraInEon eon (error "runTransactionCalculateMinValueCmd: Byron era not implemented yet") $ \sbe -> do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Byron was never supported

Copy link
Contributor

Choose a reason for hiding this comment

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

TODO can be removed now

Jimbo4350
Jimbo4350 commented Nov 14, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Options/Transaction.hs
$ Opt.progDesc "Calculate the minimum required UTxO for a transaction output."
, Just
$ pCalculateMinRequiredUtxoBackwardCompatible era
, caseByronOrShelleyBasedEra
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is to not need caseByronOrShelleyBasedEra in the future. Required byron based functionality will be hard coded and available under the byron command group.

@Jimbo4350 Jimbo4350 marked this pull request as ready for review November 14, 2023 17:09
@Jimbo4350 Jimbo4350 force-pushed the jordan/byron-isolation-1-of-n branch from d78fa54 to 053b6a1 Compare November 14, 2023 17:09
newhoggy
newhoggy reviewed Nov 16, 2023
View reviewed changes
cardano-cli/src/Cardano/CLI/Types/Common.hs
Value
TxOutDatumAnyEra
ReferenceScriptAnyEra
deriving Show
Copy link
Contributor

@newhoggy newhoggy Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We should exclude ShelleyBasedEra from all type and function names and have all the Byron specific types and functions of carr the burden of qualifying that it is only valid for ByronOnly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep 👍 . This naming is just to make it painfully obvious what I'm doing. Eventually these types won't have anything in its name to indicate the era.

@Jimbo4350
Propagate removal of ByronOnlyEra era from ...
26a2327
@Jimbo4350
runTransactionCalculateMinValueCmd was never Byron compatible
9113dfb 
Remove support for Byron by only accepting shelley based tx outputs
@Jimbo4350
Propagate minimum value calculation changes
31ad919
@Jimbo4350
Return collateral only makes sense in the context of a ShelleyBasedEra
d756cab 
specifically Babbage era onwards

Implement TxOutShelleyBasedEra and remove support for
specifying a collateral return output at a Byron address
@Jimbo4350
Propagate changes related to removing support for specifying a byron
b1fcca9 
collateral return output
@Jimbo4350
SRP: Remove me
74f5a76
@Jimbo4350
Update golden tests
ba4a419
@Jimbo4350 Jimbo4350 force-pushed the jordan/byron-isolation-1-of-n branch from b3124c4 to ba4a419 Compare November 16, 2023 13:17
This was referenced Nov 17, 2023
Closed
Merged
@Jimbo4350
Copy link
Contributor Author

Subsumed by #463

@Jimbo4350 Jimbo4350 closed this Nov 17, 2023
@Jimbo4350 Jimbo4350 deleted the jordan/byron-isolation-1-of-n branch April 12, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newhoggy newhoggy newhoggy left review comments

@carbolymer carbolymer carbolymer approved these changes

@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.
3 participants
@Jimbo4350 @newhoggy @carbolymer