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

feat: add support for multiple data script outputs when creating a token #596

Merged
merged 6 commits into from
Dec 28, 2023

Conversation

pedroferreira1
Copy link
Member

@pedroferreira1 pedroferreira1 commented Dec 18, 2023

Motivation

A use case needs to create tokens with data script outputs (more than one in the same create token transaction, if possible). This is similar to the Create NFT transaction, however we would be marking this token as an NFT in our metadata bucket, which is bad for the use case.

Because of that, we decided to add support to data script outputs in the lib methods adding them in the last positions of outputs, because the NFT standard expects the data output to be in the first output.

This is a short term solution and a new project will be created to define better how to better identify an NFT token, without fixing where the data output must be.

Acceptance Criteria

  • Add support for one or many data script outputs in a token create transaction.
    • This token can't be identified as an NFT by our wallet service lambda.
  • Change getNFTCreationFee method name to getDataScriptOutputFee.
    • Even though this is a breaking change and not critical for the new feature, this improves the method name.
  • The method to create NFT continues accepting only one data script output, so we don't change the method signature.

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@pedroferreira1 pedroferreira1 self-assigned this Dec 18, 2023
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (fcc08f6) 78.76% compared to head (8ed6944) 78.77%.

Files Patch % Lines
src/utils/tokens.ts 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #596      +/-   ##
==========================================
+ Coverage   78.76%   78.77%   +0.01%     
==========================================
  Files          68       68              
  Lines        5383     5386       +3     
  Branches     1144     1145       +1     
==========================================
+ Hits         4240     4243       +3     
  Misses       1126     1126              
  Partials       17       17              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -481,7 +483,8 @@ const tokens = {
* @param {string} [options.mintAuthorityAddress] the address to send the mint authority created
* @param {boolean} [options.createMelt=true] Whether to create a melt output
* @param {string} [options.meltAuthorityAddress] the address to send the melt authority created
* @param {string|null} [options.nftData=null] NFT data to create an NFT token
* @param {string[]|null} [options.data=null] list of data strings to add each as a data script output
Copy link
Member

Choose a reason for hiding this comment

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

question(if-minor): Do you think we should have the string be encoded?

For NFTs we used utf8 encoding since the data would be a string anyway, but now that we are allowing other types of data output should we have a different encoding, for instance base64 since its safe for utf8 strings and binary data.
Even if we keep using utf8 I think we should add the expected encoding on the docstrings, to make sure the caller knows how to pass the data.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed privately, we will keep the utf-8 encoding for now (added the info in the docs 8ed6944) and discuss this support in another issue (#597)

getNFTCreationFee(): number {
getDataScriptOutputFee(): number {
Copy link
Member

Choose a reason for hiding this comment

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

issue(if-minor): This may be used by the other wallets.

If it is being used we should remember to bump the lib and make this change on the wallets after this is released

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we would be using it in the wallets but we actually don't. We have method in the wallet utils but we should change to use the lib.

https://github.com/HathorNetwork/hathor-wallet/blob/master/src/utils/tokens.js#L111

@pedroferreira1 pedroferreira1 merged commit 6358b81 into dev Dec 28, 2023
@pedroferreira1 pedroferreira1 deleted the feat/create-token-data-output branch December 28, 2023 13:03
This was referenced Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants