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

Replace only AdaSupportedInEra and MultiAssetSupportedInEra with eons #254

Merged
merged 14 commits into from
Sep 22, 2023
Merged

Replace only AdaSupportedInEra and MultiAssetSupportedInEra with eons #254

merged 14 commits into from
Sep 22, 2023

Conversation

newhoggy
Copy link
Collaborator

@newhoggy newhoggy commented Sep 21, 2023 •
edited
Loading

Changelog

- description: |
    - Delete `AdaSupportedInEra`.  Use `ByronToAllegraEra` instead.
    - Delete `MultiAssetSupportedInEra`.  Use `MaryEraOnwards` instead.
    - Delete function `multiAssetSupportedInEra`.  Use `caseByronToAllegraOrMaryEraOnwards` instead.
    - New `ByronToAllegraEra` eon.
    - New `MaryEraOnwards` eon.
    - New functions:
      - `caseByronToAllegraOrMaryEraOnwards`
      - `caseShelleyToAllegraOrMaryEraOnwards`
      - `caseShelleyToMaryOrAlonzoEraOnwards`
      - `shelleyToAllegraEraToByronToAllegraEra`
# 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

Conversion to eons provides access to support code like constraint summoning and eon promotion.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • The change log section in the PR description has been filled in
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • round trip tests
    • integration tests
      See Running tests for more details
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-8.10.7 and ghc-9.2.7
  • The changelog section in the PR is updated to describe the change
  • Self-reviewed the diff

@newhoggy newhoggy force-pushed the newhoggy/replace-OnlyAdaSupportedInEra-and-MultiAssetSupportedInEra-with-eons branch from 8b5035d to ef8ae2c Compare September 21, 2023 11:05
@newhoggy newhoggy marked this pull request as ready for review September 21, 2023 11:12
@newhoggy newhoggy force-pushed the newhoggy/replace-OnlyAdaSupportedInEra-and-MultiAssetSupportedInEra-with-eons branch from ef8ae2c to 0976162 Compare September 21, 2023 11:20
@newhoggy newhoggy added this pull request to the merge queue Sep 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Sep 21, 2023
Jimbo4350
Jimbo4350 previously requested changes Sep 21, 2023
View reviewed changes
cardano-api/internal/Cardano/Api/Eon/ByronToAllegraEra.hs

import Data.Typeable (Typeable)

class IsByronToAllegraEra era where
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 necessary? Where is it being used. Do you have a cli branch integrated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IsByronToAllegraEra I can delete.

newhoggy
newhoggy commented Sep 21, 2023
View reviewed changes
cardano-api/internal/Cardano/Api/TxBody.hs

AdaOnlyInByronEra :: OnlyAdaSupportedInEra ByronEra
AdaOnlyInShelleyEra :: OnlyAdaSupportedInEra ShelleyEra
AdaOnlyInAllegraEra :: OnlyAdaSupportedInEra AllegraEra
Copy link
Collaborator Author

@newhoggy newhoggy Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately Byron is included here in the original feature type, which is why the replacement eon also includes Byron.

@newhoggy newhoggy force-pushed the newhoggy/replace-OnlyAdaSupportedInEra-and-MultiAssetSupportedInEra-with-eons branch from 5c0e397 to 6523019 Compare September 21, 2023 22:57
newhoggy
newhoggy commented Sep 21, 2023
View reviewed changes
cardano-api/internal/Cardano/Api/TxBody.hs
TxOutDatumNone ReferenceScriptNone


toByronTxOut :: TxOut ctx ByronEra -> Maybe Byron.TxOut
toByronTxOut (TxOut (AddressInEra ByronAddressInAnyEra (ByronAddress addr))
(TxOutAdaOnly AdaOnlyInByronEra value) _ _) =
Copy link
Collaborator Author

@newhoggy newhoggy Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We have special handling to make Byron look like Shelley. This is why we need ByronToAllegraEra.

@newhoggy newhoggy dismissed Jimbo4350’s stale review September 22, 2023 09:45

Replied to comments

@newhoggy newhoggy requested a review from Jimbo4350 September 22, 2023 09:45
@newhoggy
New ByronToAllegraEra eon
dd0153f
@newhoggy
New caseByronToAllegraOrMaryEraOnwards function
a626bd8
@newhoggy
Delete MultiAssetSupportedInEra. Use MaryEraOnwards instead
8d8b025
@newhoggy
Delete OnlyAdaSupportedInEra. Use ByronToAllegraEra instead
a3eca59
@newhoggy
Additional constraints for MaryEraOnwards and ShelleyToAllegraEra
e37dc48
@newhoggy
Delete multiAssetSupportedInEra. Use caseByronToAllegraOrMaryEraOnwar…
e0e2cf7 
…ds instead
@newhoggy
New caseShelleyToAllegraOrMaryEraOnwards and shelleyToAllegraEraToByr…
0e5c9f6 
…onToAllegraEra functions
@newhoggy
In implementation of fromShelleyTxOut, compute txOutValue and address…
b14c8bc 
…InEra once rather than in each era
@newhoggy
Delete ReferenceTxInsScriptsInlineDatumsInBabbageEra. Use BabbageEraO…
6848a82 
…nwardsConway instead.
@newhoggy
Delete refInsScriptsAndInlineDatsSupportedInEra. Use caseShelleyToAlo…
884b330 
…nzoOrBabbageEraOnwards instead.
@newhoggy
Delete ScriptDataInAlonzoEra. Use AlonzoEraOnwardsAlonzo instead
4576638
@newhoggy
New ByronToMaryEra eon
d50df2c
@newhoggy newhoggy force-pushed the newhoggy/replace-OnlyAdaSupportedInEra-and-MultiAssetSupportedInEra-with-eons branch from 6523019 to d50df2c Compare September 22, 2023 10:10
@newhoggy newhoggy mentioned this pull request Sep 22, 2023
Merged
10 tasks
@newhoggy newhoggy added this pull request to the merge queue Sep 22, 2023
Merged via the queue into main with commit fc48ece Sep 22, 2023
20 checks passed
@newhoggy newhoggy deleted the newhoggy/replace-OnlyAdaSupportedInEra-and-MultiAssetSupportedInEra-with-eons branch September 22, 2023 13:50
newhoggy added a commit that referenced this pull request Mar 11, 2024
@newhoggy
Merge pull request #254 from input-output-hk/newhoggy/remove-EraBased…
51fb8ee 
…-prefix-and-add-Cmd-suffix

Remove `EraBased` prefix and add `Cmd` suffix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

@disassembler disassembler Awaiting requested review from disassembler

@Jimbo4350 Jimbo4350 Awaiting requested review from Jimbo4350

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