Skip to content

Conversation

carbolymer
Copy link
Contributor

@carbolymer carbolymer commented Mar 7, 2025

Changelog

- description: |
    Make 1-1 relationship of witness and policy ID in TxMintValue instead of 1-*
    Remove exports: `parseValue`, `ParserValueRole`
    Add new type `PolicyAssets` representing minted assets within a single PolicyId
    Add `mkTxMintValue` helper function
# 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

Fixes:

Previous related changes to TxMintValue:

Used in:

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 marked this pull request as draft March 7, 2025 17:17
@carbolymer carbolymer self-assigned this Mar 7, 2025
@carbolymer carbolymer force-pushed the mgalazyn/refactor/pull-higher-witness-in-txmintvalue branch 2 times, most recently from 14c56ca to 38cd583 Compare March 13, 2025 16:14
@carbolymer carbolymer force-pushed the mgalazyn/refactor/pull-higher-witness-in-txmintvalue branch 2 times, most recently from f4ca29b to 7e341b2 Compare March 14, 2025 16:02
@carbolymer carbolymer force-pushed the mgalazyn/refactor/pull-higher-witness-in-txmintvalue branch 2 times, most recently from ee0da17 to 109a495 Compare March 14, 2025 16:09
@carbolymer carbolymer marked this pull request as ready for review March 14, 2025 16:10
… of 1-*

  * Remove exports: `parseValue`, `ParserValueRole`
  * Add new type `PolicyAssets` representing minted assets within a single PolicyId
  * Add `mkTxMintValue` helper function
@carbolymer carbolymer force-pushed the mgalazyn/refactor/pull-higher-witness-in-txmintvalue branch from 109a495 to ed64a00 Compare March 14, 2025 16:13
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.

One comment

Mary.scaledMinDeposit (toMaryValue v)

-- | Map of non-ADA assets with their quantity, for a single policy
newtype PolicyAssets = PolicyAssets (Map AssetName Quantity)
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost everywhere PolicyAssets is used in a Map i.e Map PolicyId PolicyAssets. Why not make this type:

newtype PolicyAssets = PolicyAssets (Map PolicyId (Map AssetName Quantity))?

And if we're doing that, we can just use the ledger type directly newtype MultiAsset = MultiAsset (Map PolicyID (Map AssetName Integer))

Copy link
Contributor Author

@carbolymer carbolymer Mar 17, 2025

Choose a reason for hiding this comment

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

I agree. But the main purpose of this type is to use it in TxMintValue where we have one witness per policy ID. So we need some type aggregating assets and quantities. You can't do it in a nice way if you have a type Map PolicyId (Map AssetName Quantity) - you'd have to keep two maps (one with quantities and the otehr with witnesses) and make sure they're in sync... cumbersome and error prone.

Copy link
Contributor Author

@carbolymer carbolymer Mar 17, 2025

Choose a reason for hiding this comment

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

An alternative would be to just use Map AssetName Quantity there, but it's annoying to handle: you'd need to write operations like merging or negating by hand. So this newtype provides MonoFunctor and Semigroup instances making those operations easier.

@carbolymer carbolymer added this pull request to the merge queue Mar 17, 2025
Merged via the queue into master with commit 3305483 Mar 17, 2025
29 checks passed
@carbolymer carbolymer deleted the mgalazyn/refactor/pull-higher-witness-in-txmintvalue branch March 17, 2025 15:45
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.

[FR] - TxMintValue shouldn't allow multiple script witnesses per PolicyId?

2 participants