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

Use the ledger's Coin instead of our custom Lovelace type #475

Merged
merged 3 commits into from
Mar 13, 2024
Merged

Use the ledger's Coin instead of our custom Lovelace type #475

merged 3 commits into from
Mar 13, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Mar 8, 2024 •
edited
Loading

Changelog

- description: |
    Use the ledger's Coin instead of our custom Lovelace type
# 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

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • Self-reviewed the diff

@smelc smelc force-pushed the smelc/newhoggy/use-L.Coin-instead-of-Lovelace branch from e5342d6 to de27351 Compare March 8, 2024 16:03
newhoggy pushed a commit that referenced this pull request Mar 11, 2024
@carbolymer
Merge pull request #475 from input-output-hk/mgalazyn/fix/revert-use-…
ca5392f 
…ledger-pp-serialization

Revert "query protocol parameters: use ledger instances"
@smelc smelc force-pushed the smelc/newhoggy/use-L.Coin-instead-of-Lovelace branch 2 times, most recently from 5f42bc4 to 623466e Compare March 11, 2024 14:45
@smelc smelc marked this pull request as ready for review March 11, 2024 15:02
@smelc smelc force-pushed the smelc/newhoggy/use-L.Coin-instead-of-Lovelace branch from 623466e to 674dfb8 Compare March 11, 2024 15:04
@smelc smelc force-pushed the smelc/newhoggy/use-L.Coin-instead-of-Lovelace branch from 674dfb8 to 46c1e60 Compare March 11, 2024 16:00
carbolymer
carbolymer reviewed Mar 11, 2024
View reviewed changes
...iles/golden/errors/Cardano.Api.Fees.TxBodyErrorAutoBalance/TxBodyErrorAdaBalanceNegative.txt Outdated
The transaction does not balance in its use of ada. The net balance of the transaction is negative: Coin 1 lovelace. The usual solution is to provide more inputs, or inputs with more ada.
Copy link
Contributor

@carbolymer carbolymer Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This error message reads odd ... Coin 1 lovelace. Can we add Pretty instance for L.Coin and use it for human-readable printing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the two additional commits 👍

carbolymer
carbolymer reviewed Mar 13, 2024
View reviewed changes
cardano-api/internal/Cardano/Api/Orphans.hs Outdated
@@ -189,6 +190,8 @@ deriving instance Data Bech32.CharPosition
deriving newtype instance Real L.Coin
deriving newtype instance Integral L.Coin
deriving newtype instance Num L.Coin
instance Pretty L.Coin where
pretty (L.Coin n) = pretty n <+> "lovelace"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pretty (L.Coin n) = pretty n <+> "lovelace"
pretty (L.Coin n) = pretty n <+> "Lovelace"

Lovelace is capitalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, changed 👍

@smelc smelc force-pushed the smelc/newhoggy/use-L.Coin-instead-of-Lovelace branch from 544541e to cc73f18 Compare March 13, 2024 12:59
@smelc smelc enabled auto-merge March 13, 2024 12:59
@smelc smelc added this pull request to the merge queue Mar 13, 2024
Merged via the queue into main with commit 048a1ab Mar 13, 2024
23 of 24 checks passed
@smelc smelc deleted the smelc/newhoggy/use-L.Coin-instead-of-Lovelace branch March 13, 2024 13:26
@carbolymer carbolymer mentioned this pull request Mar 13, 2024
Closed
3 tasks
Jimbo4350
Jimbo4350 reviewed Mar 27, 2024
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.

👍 We should have probably deprecated the Lovelace type with a warning and then removed it fully after a release.

cardano-api/internal/Cardano/Api/Value.hs
@@ -47,15 +47,11 @@ module Cardano.Api.Value
-- * Internal conversion functions
, toByronLovelace
, fromByronLovelace
, toShelleyLovelace
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@smelc smelc mentioned this pull request Apr 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 left review comments

@carbolymer carbolymer carbolymer approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@newhoggy newhoggy Awaiting requested review from newhoggy

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

@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.

Remove the Lovelace type. Use the ledger type instead.
3 participants
@smelc @carbolymer @Jimbo4350