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.

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

Simplify multi-asset value conversions. #4125

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

jonathanknowles
Copy link
Member

@jonathanknowles jonathanknowles commented Sep 13, 2023

Issue

None. Noticed while browsing the code.

Summary

This PR makes the following simplification in several places:

- fromCardanoValue . Cardano.fromMaryValue
+ toWalletTokenBundle

The original expression performs a two-step conversion of multi-asset (MA) values:

  1. From a cardano-ledger MA value (nested map) to a cardano-api MA value (flat map)
  2. From a cardano-api MA value (flat map) to a cardano-wallet MA value (nested map)

The toWalletTokenBundle function performs this conversion in a single step, and doesn't require the creation of an intermediate cardano-api MA value.

In particular, we simplify expressions of the following form:

- `fromCardanoValue . Cardano.fromMaryValue`

The above expression peforms the following two-step conversion:

- From a ledger MA value (nested map) to a cardano-api MA value (flat map)
- From a cardano-api MA value (flat map) to a wallet MA value (nested map)

The `toWalletTokenBundle` function performs this conversion in a single step.
In particular, we simplify expressions of the following form:

- `fromCardanoValue . Cardano.fromMaryValue`

The above expression peforms the following two-step conversion:

- From a ledger MA value (nested map) to a cardano-api MA value (flat map)
- From a cardano-api MA value (flat map) to a wallet MA value (nested map)

The `toWalletTokenBundle` function performs this conversion in a single step.
@jonathanknowles jonathanknowles changed the title Simplify MA value conversions. Simplify multi-asset value conversions. Sep 13, 2023
@jonathanknowles jonathanknowles self-assigned this Sep 13, 2023
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

This may risk entangling the wallet and cardano-balance-tx more, but it may be worth it as it is better 🤔

@jonathanknowles
Copy link
Member Author

jonathanknowles commented Sep 13, 2023

This may risk entangling the wallet and cardano-balance-tx more, but it may be worth it as it is better 🤔

I was thinking it could make things easier, as the dependency set of this particular conversion has been reduced in the following way:

- {cardano-wallet-primitive, cardano-ledger, cardano-api}
+ {cardano-wallet-primitive, cardano-ledger}

In other words, if we have a ledger value, or a primitive value, we'd no longer need to go via the equivalent cardano-api value when converting.

OTOH, much of the balanceTransaction API makes use of cardano-api types. Perhaps it would be worth having a discussion on which set of types we'd prefer to depend upon?

Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

On second thought I really don't think there's a meaningful drawback with this in particular, but happy to discuss tomorrow anyway. My concern would be where Compatibility.Ledger would live now / in the future and who'd depend on it.

@jonathanknowles jonathanknowles added this pull request to the merge queue Sep 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 14, 2023
@jonathanknowles jonathanknowles added this pull request to the merge queue Sep 14, 2023
Merged via the queue into master with commit 49393a6 Sep 14, 2023
3 checks passed
@jonathanknowles jonathanknowles deleted the jonathanknowles/simplify-value-conversions branch September 14, 2023 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants