Skip to content

Remove partial instances #842

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
Merged
carbolymer merged 2 commits into from
Jun 5, 2025
Merged

Remove partial instances #842

carbolymer merged 2 commits into from
Jun 5, 2025

Conversation

@carbolymer
Copy link
Contributor

@carbolymer carbolymer commented May 15, 2025 •
edited
Loading

Changelog

- description: |
    Remove partial `IsString` instances. Use `deserialiseFromRawBytesHex` or specialised parser instead.
    Make `SerialiseAsBech32` class use `HumanReadablePart` instead of `Text` for Bech32 prefix in `bech32PrefixFor` and `bech32PrefixesPermitted` functions.
# 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

IsString instances generated from UsingRawBytesHex were partial and could fail with runtime exception leading to hard to track errors. We were using fromString on types using those instances in cardano-cli and cardano-testnet already. This PR removes those and adds parsec parsers instead.

Because IsString was used in tests, it's replaced by specialised functions in tests only for example:

mkCredential :: HasCallStack => Text -> L.Credential k
mkCredential = errorFail @String . L.parseCredential

mkTxIn :: HasCallStack => Text -> TxIn
mkTxIn = either error id . P.runParser parseTxIn

ledger's parseCredential

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 self-assigned this May 15, 2025
@carbolymer carbolymer force-pushed the mgalazyn/refactor/remove-partial-instances branch 4 times, most recently from 2525da4 to 3609462 Compare May 27, 2025 15:03
github-advanced-security[bot]
github-advanced-security bot found potential problems May 27, 2025
View reviewed changes
@carbolymer carbolymer force-pushed the mgalazyn/refactor/remove-partial-instances branch from 3609462 to 7868396 Compare May 27, 2025 18:56
github-advanced-security[bot]
github-advanced-security bot found potential problems May 27, 2025
View reviewed changes
@carbolymer carbolymer force-pushed the mgalazyn/refactor/remove-partial-instances branch 6 times, most recently from c7858b6 to 10520e6 Compare May 29, 2025 07:21
@carbolymer carbolymer force-pushed the mgalazyn/refactor/remove-partial-instances branch 6 times, most recently from 6eec886 to a4f884c Compare May 29, 2025 09:44
@carbolymer carbolymer marked this pull request as ready for review May 29, 2025 09:45
@carbolymer carbolymer force-pushed the mgalazyn/refactor/remove-partial-instances branch 5 times, most recently from 24f9066 to 7a1091f Compare May 29, 2025 10:27
@carbolymer carbolymer mentioned this pull request May 29, 2025
Merged
9 tasks
@carbolymer carbolymer force-pushed the mgalazyn/refactor/remove-partial-instances branch 6 times, most recently from 60d0690 to 4e47f25 Compare June 3, 2025 10:02
newhoggy
newhoggy reviewed Jun 3, 2025
View reviewed changes
cardano-api/src/Cardano/Api/Internal/SerialiseRaw.hs
deriving Show

newtype SerialiseAsRawBytesError = SerialiseAsRawBytesError
-- TODO We can do better than use String to carry the error message
Copy link
Collaborator

@newhoggy newhoggy Jun 3, 2025

Choose a reason for hiding this comment

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

👍

newhoggy
newhoggy reviewed Jun 3, 2025
View reviewed changes
cardano-api/test/cardano-api-test/Test/Cardano/Api/Transaction/Autobalance.hs
L.RewardAccount
{ L.raNetwork = L.Testnet
, L.raCredential = L.KeyHashObj "0b1b872f7953bccfc4245f3282b3363f3d19e9e001a5c41e307363d7"
, L.raCredential = mkCredential "keyHash-0b1b872f7953bccfc4245f3282b3363f3d19e9e001a5c41e307363d7"
Copy link
Collaborator

@newhoggy newhoggy Jun 3, 2025

Choose a reason for hiding this comment

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

Can you say a bit more about this change in the PR description?

Copy link
Contributor Author

@carbolymer carbolymer Jun 5, 2025

Choose a reason for hiding this comment

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

Added.

Jimbo4350
Jimbo4350 approved these changes Jun 4, 2025
View reviewed changes
cardano-api/src/Cardano/Api/Internal/SerialiseBech32.hs
class (HasTypeProxy a, SerialiseAsRawBytes a) => SerialiseAsBech32 a where
-- | The human readable prefix to use when encoding this value to Bech32.
bech32PrefixFor :: a -> Text
bech32PrefixFor :: a -> Bech32.HumanReadablePart
Copy link
Contributor

@Jimbo4350 Jimbo4350 Jun 4, 2025

Choose a reason for hiding this comment

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

Be sure to include this breaking change in the PR description so it is obvious when generating the change logs in the next release.

Copy link
Contributor Author

@carbolymer carbolymer Jun 5, 2025

Choose a reason for hiding this comment

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

Included in the changelog entry.

cardano-api/cardano-api.cabal
filepath,
hedgehog >=1.1,
hedgehog-extras ^>=0.7,
microlens,
Copy link
Contributor

@Jimbo4350 Jimbo4350 Jun 4, 2025

Choose a reason for hiding this comment

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

@palas at some point in the future we should do an audit of our dependencies in order to reduce the size of our wasm library.

Copy link
Contributor Author

@carbolymer carbolymer Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is microlens problematic here?

Copy link
Contributor Author

@carbolymer carbolymer Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

On the note of putting cardano-api on a diet - we have both parsec and attoparsec in dependencies. I'm not sure if both are needed.

cardano-api/src/Cardano/Api/Internal/Keys/Shelley.hs Outdated
error "castVerificationKey (DRep): byron and shelley key sizes do not match!"

-- | Parse hex representation of any 'Hash'
parseHexHash :: P.Parser (Hash PaymentKey)
Copy link
Contributor

@Jimbo4350 Jimbo4350 Jun 4, 2025

Choose a reason for hiding this comment

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

parseHexPaymentKeyHash? Many different things can be hex encoded.

Copy link
Contributor Author

@carbolymer carbolymer Jun 5, 2025

Choose a reason for hiding this comment

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

Typo, I meant to write parseHexHash :: SerialiseAsRawBytes (Hash a) => P.Parser (Hash a). Thanks for spotting that.

cardano-api/src/Cardano/Api/Internal/Script.hs
]
parseRawBytesHex
where
-- ADDRHASH = Blake2b_224
Copy link
Contributor

@Jimbo4350 Jimbo4350 Jun 4, 2025

Choose a reason for hiding this comment

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

👍

@carbolymer carbolymer force-pushed the mgalazyn/refactor/remove-partial-instances branch from 4e47f25 to 7f550f7 Compare June 5, 2025 17:01
@carbolymer carbolymer force-pushed the mgalazyn/refactor/remove-partial-instances branch from 7f550f7 to 68505ed Compare June 5, 2025 17:11
@carbolymer carbolymer enabled auto-merge June 5, 2025 17:11
@carbolymer carbolymer mentioned this pull request Jun 5, 2025
Merged
3 tasks
@carbolymer carbolymer added this pull request to the merge queue Jun 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 5, 2025
@carbolymer carbolymer added this pull request to the merge queue Jun 5, 2025
Merged via the queue into master with commit e84d098 Jun 5, 2025
25 of 28 checks passed
@carbolymer carbolymer deleted the mgalazyn/refactor/remove-partial-instances branch June 5, 2025 19:45
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 approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts dcoutts is a code owner

@erikd erikd Awaiting requested review from erikd erikd is a code owner

@palas palas Awaiting requested review from palas palas is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara CarlosLopezDeLara is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Assignees

@carbolymer carbolymer

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@carbolymer @newhoggy @Jimbo4350