[Fix] merge metadata by tag

[d-Bharti001]

Changes:

• meshTxBuilderBody.metadata is changed to a map instead of an array
• Transaction's setMetadata():
  • Parameter names changed to the standard "label" and "metadata"
  • New optional parameter added to recursively merge new metadata with the existing metadata under the same label
• MeshTxBuilder's metadataValue():
  • Parameter "tag" changed to "label" with preferred type number
  • New optional parameter... same as the above
• Wherever the metadataValue() function is called, the first parameter is changed to a number type. Documents as well as Mesh's other components

Fixes:

• preserve metadata with multiple assets minting

Summary

The problem

On building a transaction (tx.build() or txBuilder.complete()), some of the transaction metadata was getting lost.

In the metadata array (meshTxBuilderBody.metadata), multiple entries (of type { tag: string, metadata: string }) with the same tag can be present.
When csl.js_serialize_tx_body() is called to get txBuildResult in line 72 of mesh-core-csl/src/core/serializer.ts (inside the function serializeTxBody of CSLSerializer)

const txBuildResult = csl.js_serialize_tx_body(txBodyJson, params);

the serializer function overrides the metadata of a tag in case there are multiple entries belonging to the same tag.

An example when this metadata-loss becomes an obvious problem is when multiple assets are minted using Transaction.mintAsset() (e.g. minting multiple CIP-25 NFTs under the same policy ID in one transaction).
meshTxBuilderBody.metadata array contains entries for all the assets, but when the transaction is built at the end, only the metadata of the last asset remains, and all of the rest are discarded by the serializer.
To avoid losing metadata, the user has to manually build a metadata object in their code and set the transaction's metadata by calling tx.setMetadata(721, metadataObj). But this also wastes the action of the Transaction.mintAsset() to automatically add the asset metadata.

The solution

-- Updated solution --
Updated metadata structure
As suggested by a maintainer, meshTxBuilderBody.metadata is updated to be a Map now (type TxMetadata)

// Transaction Metadata

export type MetadatumMap = Map<Metadatum, Metadatum>;
export type Metadatum = bigint | number | string | Uint8Array | MetadatumMap | Metadatum[];
export type TxMetadata = Map<bigint, Metadatum>;

// to be used for serialization
export type Metadata = {
  tag: string;
  metadata: string;
};

Earlier it was of type Metadata[], which is now being used only inside the serializer (mesh-core-csl/src/core/serializer.ts --> CSLSerializer --> serializeTxBody()), i.e. by calling meshTxBuilderBodyToObj() (from inside serializeTxBody()), the metadata is converted to the earlier type of [{ tag: string, metadata: <JSONbig stringified item> }]

Metadata merge feature
Merging of metadata is supported in a recursive manner upto a specified depth. For merging CIP-25 standard metadata, this depth should be set to 2.

• The first level (depth) combines different policy IDs into one object
• The second level combines different asset IDs under the same policy ID into one object

When tx.mintAsset() is called, the metadata of the current asset is merged with the existing metadata under the related label with the strategy described above.

Added functionality

There is an additional possibility added to both Transaction.setMetadata() and MeshTxBuilder.metadataValue():

• An optional third argument, when supplied, tells if the metadata has to be merged with the existing one, and also upto what depth

Example implementation of the added functionality:

• I want to mint NFTs as per the CIP-25 standard.
• After calling tx.mintAsset() one or more number of times, I want to add a version.
• I can simply merge my version with the existing 721 metadata by calling:

  tx.setMetadata(721, { version: 1 }, true)

• The third argument true sets the merge depth to 1 by default (a number can also be supplied directly as the required merge depth). So, the existing metadata object under the tag 721 (which is currently containing all the policy IDs) gets one more key-value pair, i.e. version: "1.0". Now the 721 metadata contains all the policy IDs, all NFTs, and a version.
• If a version was specified before, setting it again will replace it with the new one (because the merge depth is set to 1 here).

Affect components

• @meshsdk/common
• @meshsdk/contract
• @meshsdk/core
• @meshsdk/core-csl
• @meshsdk/core-cst
• @meshsdk/provider
• @meshsdk/react
• @meshsdk/svelte
• @meshsdk/transaction
• @meshsdk/wallet
• Mesh playground (i.e. https://meshjs.dev/)
• Mesh CLI

Type of Change

• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Bug fix (non-breaking change which fixes an issue)
• Code refactoring (improving code quality without changing its behavior)
• Documentation update (adding or updating documentation related to the project)

Related Issues

Not any

Checklist

Please ensure that your pull request meets the following criteria:

• My code is appropriately commented and includes relevant documentation, if necessary
• I have added tests to cover my changes, if necessary
• I have updated the documentation, if necessary
• All new and existing tests pass (i.e. npm run test)
• The build is pass (i.e. npm run build)

Additional Information

For compatibility with the previous codes which used Mesh library, these steps are taken:

• MeshTxBuilder.metadataValue()
  • As the input label (tag) is now preferred as a Number, which was String earlier, it is now explicitly converted to a BigInt now. String inputs would also work.
• Codes which explicitly set a metadata value
  • As they don't pass any 3rd argument, the already existing Metadata under the concerned label would automatically get replaced. No behaviour is changed here.

For compatibility with transaction serialization, the new Map metadata is still converted to the same old type Metadata[]. So no change should be seen in the final transaction body built by tx.build() or txBuilder.complete().

Have a look at the unit test cases: packages/mesh-transaction/test/transaction/txMetadata.test.ts

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mesh-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 16, 2024 7:40am
mesh-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 16, 2024 7:40am

[vercel]

@d-Bharti001 is attempting to deploy a commit to the MeshJS Team on Vercel.

A member of the Team first needs to authorize it.

[twwu123]

As far as I know, the function call at csl.js_serialize_tx_body actually doesn't care about multiple entries with the same tag. The actual issue is simply that, the serializer calls set_metadata multiple times in the case of having multiple metadata fields, which means it will keep overwriting previous metadata elements.

Edit: I was wrong about this, the serializer actually does insertion with a key based on tag.

Unfortunately, this issue actually stems from the maintainers of this library being several different people with different design philosophies. The idea from the serializer perspective, is that what's being called is set_metadata and so the behaviour makes sense to completely erase any previously set metadata.

However, the design philosophy of the Transaction class preferred to add multiple fields to the metadata on each call of Transaction.mintAsset(). My preferred solution is actually to remove this behaviour from the Transaction class completely. Force users to build the exact structure of their metadata and set it in one go.

The reason I prefer this, is because when there are multiple metadata fields, you have to start guessing how the users want to combine them. This is the reason why I would be reluctant to approve this PR, while this merging of metadata tags may work for your structure of metadata, it is still a guess on how the user wants to structure their metadata.

How would you feel if we simply removed this behaviour of the Transaction class and force the entire metadata to be set in one go? @d-Bharti001

[twwu123]

Upon further thought and investigation, your solution seems more and more reasonable. I'd like to point some things out, to see if we can gain some insight into what's the best solution.

I still think there's something fundamentally wrong with the current implementation of Metadatum in the txBuilder. The trouble is that the resulting Metadatum is a mapping, while the Metadatum field in the txBuilder is a list, which ultimately makes them difficult to resolve. The conversion from a list to a map inherently requires some guesswork as to how to combine the list.

My proposal would be to change this type into an actual map, taking the implementation of cardano-js-sdk

export declare type MetadatumMap = Map<Metadatum, Metadatum>;
export declare type Metadatum = bigint | MetadatumMap | string | Uint8Array | Metadatum[];
export declare type TxMetadata = Map<bigint, Metadatum>;

We could then replace the current Metadata[] with TxMetadata

This would require some work to overhaul the current Transaction and TxBuilder classes, but I think would make more sense ultimately.

[d-Bharti001]

@twwu123 I agree with your second comment, where TxMetadata is a mapping. We would need that merge-metadata functionality anyway.

• If we just require setting the metadata totally from the user's side and not do anything inside the mintAsset function, then the users who have written codes for minting only one token in a transaction would need to upgrade their code.
• Other functions of Transaction also call the set-metadata function.

One question.. should the key be a bigint, or a string?

export declare type TxMetadata = Map<bigint, Metadatum>;

Does that bigint automatically get converted to string in the final transaction's metadata? If that's the case, bigint is fine.

Should I proceed with making this upgrade? If you have any suggestions and inputs, e.g. regarding the names of functions and arguments, please share.

[d-Bharti001]

I'll start working on this change.

[twwu123]

I think the key should be a string, on Rust side, the serializer calls BigNum::from_str(&metadata.tag).

I personally think that making breaking changes is fine here, because the old method of setting metadatums has been an issue for some time, as you pointed out, multiple metadata of the same tag get replaced. It's been reported by quite a few users at this point, so it would be good to overhaul the implementation in order to make it more permanent.

Setting metadata in one go makes a lot more sense in my opinion, it results in no ambiguity in reconciling a list of metadata, and gives users fine control over how they want to structure it.

Thanks a lot for this, this work is really appreciated.

[d-Bharti001]

@twwu123 cool! I'm on it.

[d-Bharti001]

Hi @twwu123 I've pushed the updated code where tx. metadata is a Map. I agree with the point that the user should prepare the whole metadata from their side, which is a lot clearer and straigtforward. However, for simple tasks like if I'm minting multiple NFTs by calling tx.mintAsset() without caring about how the metadata is being built, and then I just want to set a version number at last, merge feature seems useful to me as I can do it simply like this: tx.setMetadata(721, { version: 1 }, true).
As this feature might confuse users, I'm open to changes.

[twwu123]

Thank you, it looks mostly good, will be looking in more detail in this week and get this merged.

[d-Bharti001]

Hi @twwu123 thanks a lot for reviewing and doing the necessary changes.

In the function metadataObjToMap(), I've added a case for Map, that I had forgot earlier. Please check (packages/mesh-transaction/src/utils/metadata.ts --> lines 22 to 31)

[d-Bharti001]

Hi @twwu123 I have one concern:
In the Transaction's mintAsset() function:

if (mint.label === "721" || mint.label === "20") {
    let currentMetadata = this.txBuilder.meshTxBuilderBody.metadata;
    if (currentMetadata.size === 0) {
        this.setMetadata(Number(mint.label), {
           [policyId]: { [mint.assetName]: mint.metadata },
        });
    } else {
        let metadataMap = metadataObjToMap({
            [policyId]: { [mint.assetName]: mint.metadata },
        } as object);
        let newMetadata = mergeContents(
            currentMetadata.get(BigInt(mint.label)) as Metadatum,
            metadataMap,
            mint.label === "721" ? 2 : 0,
        );
        this.setMetadata(Number(mint.label), newMetadata);
    }
}

mergeContents() is used in line 484. Would it work if currentMetadata.get(BigInt(mint.label)) returns undefined, i.e. when metadata map doesn't contain any entry of key 721?

P.S.: I don't have much experience with TypeScript

[twwu123]

That's a good point, it probably wouldn't work

[twwu123]

@d-Bharti001

if (currentMetadata.get(BigInt(mint.label)) === undefined) {
          this.setMetadata(Number(mint.label), {
            [policyId]: { [mint.assetName]: mint.metadata },
          });
        } else {
          let metadataMap = metadataObjToMap({
            [policyId]: { [mint.assetName]: mint.metadata },
          } as object);
          let newMetadata = mergeContents(
            currentMetadata.get(BigInt(mint.label)) as Metadatum,
            metadataMap,
            mint.label === "721" ? 2 : 0,
          );
          this.setMetadata(Number(mint.label), newMetadata);
        }

This would be way better, that's my mistake

Thanks for pointing it out

[d-Bharti001]

@twwu123 that looks good! thanks.