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

Propagate protocol parameters conversion errors #5197

Merged
merged 1 commit into from
May 25, 2023
Merged

Propagate protocol parameters conversion errors #5197

merged 1 commit into from
May 25, 2023

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented May 5, 2023 •
edited
Loading

Description

This PR introduces an error sum type: ProtocolParametersConversionError and propagation of this type allowing for printing and handling of conversion errors, without using error, as it was previously.

This PR is blocked by a release of the following change in cardano-api: IntersectMBO/cardano-api#1

closes #5092

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Runnings tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • 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
  • Self-reviewed the diff

@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch from 0000b3a to 8bfcb7f Compare May 5, 2023 14:20
@carbolymer carbolymer mentioned this pull request May 5, 2023
Merged
11 tasks
@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch from 8bfcb7f to 456e729 Compare May 5, 2023 15:27
@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch 3 times, most recently from 08833df to ddceb63 Compare May 9, 2023 19:07
Jimbo4350
Jimbo4350 reviewed May 10, 2023
View reviewed changes
cardano-api/src/Cardano/Api/Fees.hs Outdated
bpparams = bundleProtocolParams era' pparams

bpparams'e :: Either TxBodyErrorAutoBalance (BundledProtocolParameters era)
bpparams'e = first (TxBodyError . TxBodyProtocolParamsConversionError) $ bundleProtocolParams era' pparams
Copy link
Contributor

@Jimbo4350 Jimbo4350 May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Using special characters (in this case the apostrophe) in variable names make them harder to read and understand. I was actually confused at first as I thought ' was some kind of operator.

Copy link
Contributor Author

@carbolymer carbolymer May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's a one way of encoding that the variable in scope is similar to the other one, but it's wrapped in Either. I've changed to prefix e to match the convention how we mark Maybes.

Jimbo4350
Jimbo4350 reviewed May 10, 2023
View reviewed changes
cardano-api/src/Cardano/Api/TxBody.hs Outdated
@@ -3736,20 +3741,20 @@ makeShelleyTransactionBody era@ShelleyBasedEraMary
txMintValue
} = do

validateTxBodyContent era txbodycontent
Copy link
Contributor

Choose a reason for hiding this comment

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

So you have introduced a lot of unnecessary diffs by changing the indent which makes this PR harder to review because there are now semantic and non-semantic changes. Please undo the indent changes. I know the formatting in cardano-api does not adhere to the style document but if we want to change that, it's best to do that in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, indentation restored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possible to view by adding ?w=1 to the URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/input-output-hk/cardano-node/pull/5197/files?w=1

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I never knew that

@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch 2 times, most recently from ba26b3b to c1f8734 Compare May 10, 2023 17:05
@carbolymer carbolymer marked this pull request as ready for review May 10, 2023 17:10
@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch from c1f8734 to b78ed2c Compare May 10, 2023 17:15
@newhoggy
Copy link
Contributor

Conflicts

@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch 4 times, most recently from 343caaf to cbc5714 Compare May 11, 2023 11:57
newhoggy
newhoggy approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@newhoggy newhoggy left a comment

Choose a reason for hiding this comment

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

LGTM

@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch from cbc5714 to e7c17c7 Compare May 16, 2023 15:33
@hamishmack hamishmack force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch from e7c17c7 to 8db0d36 Compare May 17, 2023 00:29
@newhoggy
Copy link
Contributor

Where was the call to error that was removed? I can't find it. If it's there can you comment on it so I can see where it is?

newhoggy
newhoggy reviewed May 17, 2023
View reviewed changes
cardano-api/src/Cardano/Api/ProtocolParameters.hs Outdated
toLedgerPParamsUpdate :: ShelleyBasedEra era
-> ProtocolParametersUpdate
-> Ledger.PParamsUpdate (ShelleyLedgerEra era)
toLedgerPParamsUpdate sbe = either error id . toLedgerPParamsUpdateEither sbe
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh found it!

newhoggy
newhoggy reviewed May 17, 2023
View reviewed changes
cardano-api/src/Cardano/Api/ProtocolParameters.hs Outdated
toLedgerPParams
:: ShelleyBasedEra era
-> ProtocolParameters
-> Ledger.PParams (ShelleyLedgerEra era)
toLedgerPParams era = either error id . toLedgerPParamsEither era
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, there's two.

@carbolymer carbolymer mentioned this pull request May 17, 2023
Merged
11 tasks
Jimbo4350
Jimbo4350 reviewed May 17, 2023
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.

Feel free to merge once John is happy.

@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch 2 times, most recently from 2e9e99a to d04f655 Compare May 18, 2023 12:28
@carbolymer carbolymer mentioned this pull request May 18, 2023
Merged
9 tasks
@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch 2 times, most recently from 492eef8 to 5bb7148 Compare May 18, 2023 14:58
@carbolymer carbolymer requested review from a team, coot, deepfire and jutaro as code owners May 18, 2023 14:58
@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch 5 times, most recently from b705d40 to 246af1b Compare May 25, 2023 10:22
@carbolymer carbolymer requested review from mgmeier and fmaste as code owners May 25, 2023 10:22
@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch from 246af1b to fa73b87 Compare May 25, 2023 11:52
@carbolymer carbolymer enabled auto-merge May 25, 2023 11:55
@carbolymer carbolymer force-pushed the mgalazyn/feature/error-handling-for-pp-conversion branch from fa73b87 to ce6891b Compare May 25, 2023 14:16
@carbolymer carbolymer added this pull request to the merge queue May 25, 2023
Merged via the queue into master with commit 186b530 May 25, 2023
@iohk-bors iohk-bors bot deleted the mgalazyn/feature/error-handling-for-pp-conversion branch May 25, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newhoggy newhoggy newhoggy approved these changes

@Jimbo4350 Jimbo4350 Jimbo4350 left review comments

@disassembler disassembler disassembler approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

@coot coot Awaiting requested review from coot

@deepfire deepfire Awaiting requested review from deepfire

@jutaro jutaro Awaiting requested review from jutaro

@mgmeier mgmeier Awaiting requested review from mgmeier

@fmaste fmaste Awaiting requested review from fmaste

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

Successfully merging this pull request may close these issues.

[FR] - Propagate PParams conversion errors
4 participants
@carbolymer @newhoggy @disassembler @Jimbo4350