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

Introduce EraBasedProtocolParametersUpdate #180

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Introduce EraBasedProtocolParametersUpdate #180

merged 1 commit into from
Aug 11, 2023

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Aug 9, 2023 •
edited
Loading

Changelog

- description: |
    Implement `EraBasedProtocolParametersUpdate`
# 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.

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

Jimbo4350
Jimbo4350 commented Aug 9, 2023
View reviewed changes
cardano-api/internal/Cardano/Api/ProtocolParameters.hs Outdated
let common = createCommonPParamsUpdate c
preAl' = createPParamsUpdatePreAlonzo preAl
pCon' = createPParamsUpdatePreConway pCon
in common -- <> preAl' <> pCon'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A SemiGroup PParams instance will allow us to do this.

newtype PParamsUpdate era = PParamsUpdate (PParamsHKD StrictMaybe era)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this something ledger can provide? Or are we proposing to add a wrapper?

Copy link
Contributor

@carbolymer carbolymer Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we could use PParams instead of PPUpdate here, it should be possible to do:

 L.emptyPParams `L.applyPPupdates` preAl' `L.applyPPupdates` pCon'

but then PParams is a bit unwieldy here

Copy link
Contributor Author

@Jimbo4350 Jimbo4350 Aug 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this something ledger can provide? Or are we proposing to add a wrapper?

They can provide it but I'm going to do an orphan instance.

If we could use PParams instead of PPUpdate here, it should be possible to do:
L.emptyPParams L.applyPPupdates preAl' L.applyPPupdates pCon'
but then PParams is a bit unwieldy here

This is a little hacky.

cardano-api/internal/Cardano/Api/ProtocolParameters.hs Outdated

data IntroducedInConwayPParamsUpdate era

data IntroducedInAlonzoPParamsUpdate ledgerera
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicit data type that tells us what was introduced in the Alonzo era

cardano-api/internal/Cardano/Api/ProtocolParameters.hs Outdated
, alMaxCollateralInputs :: StrictMaybe Natural
}

data CommonProtocolParameters
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently all of these are common to all Shelley based eras. We can pluck them as necessary if any are deprecated.

cardano-api/internal/Cardano/Api/ProtocolParameters.hs Outdated
(StrictMaybe Ledger.Nonce) -- ^ Extra entropy
(StrictMaybe Ledger.UnitInterval) -- ^ Decentralization parameter

createPParamsUpdatePreConway
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additional type safety as a result of using ledger's lenses

@Jimbo4350 Jimbo4350 mentioned this pull request Aug 10, 2023
Closed
10 tasks
@Jimbo4350 Jimbo4350 force-pushed the jordan/use-ledger-pparams branch from c1955e9 to a8e8a28 Compare August 10, 2023 21:17
@Jimbo4350 Jimbo4350 changed the title EraBasedProtocolParameters Introduce EraBasedProtocolParametersUpdate Aug 10, 2023
@Jimbo4350 Jimbo4350 marked this pull request as ready for review August 10, 2023 21:19
@Jimbo4350 Jimbo4350 force-pushed the jordan/use-ledger-pparams branch from a8e8a28 to 5c25aa3 Compare August 10, 2023 21:22
newhoggy
newhoggy reviewed Aug 10, 2023
View reviewed changes
cardano-api/internal/Cardano/Api/ProtocolParameters.hs
data DeprecatedAfterAlonzoPParams' ledgerera
= DeprecatedAfterAlonzoPParams'
(StrictMaybe Ledger.Nonce) -- ^ Extra entropy
(StrictMaybe Ledger.UnitInterval) -- ^ Decentralization parameter
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is a good case to use field names we we break up the protocol parameters into pieces like this.

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 tried to add record but GHC is complaining they aren't used.

newhoggy
newhoggy reviewed Aug 11, 2023
View reviewed changes
cardano-api/internal/Cardano/Api/ProtocolParameters.hs

-- | Each constructor corresponds to the set of protocol parameters available
-- in a given era.
data EraBasedProtocolParametersUpdate era where
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will we be needing another one for EraBasedProtocolParameters era to match PParams 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.

I want to reuse this to construct PParams era

@Jimbo4350 Jimbo4350 force-pushed the jordan/use-ledger-pparams branch 8 times, most recently from cccd834 to 7298f0c Compare August 11, 2023 19:47
@Jimbo4350 Jimbo4350 force-pushed the jordan/use-ledger-pparams branch from 7298f0c to aa68252 Compare August 11, 2023 20:02
@Jimbo4350 Jimbo4350 enabled auto-merge August 11, 2023 20:03
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Aug 11, 2023
Merged via the queue into main with commit 2fa7b1b Aug 11, 2023
@Jimbo4350 Jimbo4350 deleted the jordan/use-ledger-pparams branch August 11, 2023 20:28
newhoggy added a commit that referenced this pull request Mar 11, 2024
@newhoggy
Merge pull request #180 from input-output-hk/newhoggy/remove-example-…
45d9b55 
…era-based-parsers

Remove example era-based parsers we don't need anymore
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

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@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