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 new transaction of Mint type #604

Merged
merged 11 commits into from
Nov 17, 2022

Conversation

LuizAsFight
Copy link
Contributor

@LuizAsFight LuizAsFight commented Nov 16, 2022

Add new Mint transaction

  • decoders/encoders
  • fix TransactionResponse to work with few props (no receipts, gas, etc..)
  • specify better typing in TransactionRequest, as we don't need/should to use Mint transactions in there
  • add better typing for Transaction, to improve usage when consuming TransactionResponse / TransactionResult in frontend side
    • now a Transaction will have all possible parameters as optional.
    • Transaction<TransactionType.Script> will have the exact params for Script transaction only
    • Transaction<TransactionType.Create> will have the exact params for Create transaction only
    • Transaction<TransactionType.Mint> will have the exact params for Mint transaction only

Closes #603
Closes #602

@github-actions
Copy link
Contributor

github-actions bot commented Nov 16, 2022

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
90.66% (+0.05% 🔼)
3699/4080
🟡 Branches
72.5% (+0.01% 🔼)
717/989
🟢 Functions
87.5% (+0.04% 🔼)
735/840
🟢 Lines
90.71% (+0.05% 🔼)
3546/3909
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / transaction-response.ts
82.5%
54.55% (-1.01% 🔻)
100% 82.5%

Test suite run success

549 tests passing in 49 suites.

Report generated by 🧪jest coverage report action from 38ecb79

@LuizAsFight LuizAsFight requested a review from a team November 17, 2022 19:51
@LuizAsFight LuizAsFight marked this pull request as ready for review November 17, 2022 19:52
@@ -291,7 +294,62 @@ export class TransactionCreateCoder extends Coder<TransactionCreate, Transaction
}
}

export type Transaction = TransactionScript | TransactionCreate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate on why this was removed? I think we can do TransactionScript | TransactionCreate | TransactionMint, but I may be missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it wasn't removed, it's just ahead in the code

Copy link
Contributor Author

@LuizAsFight LuizAsFight Nov 17, 2022

Choose a reason for hiding this comment

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

TransactionScript | TransactionCreate | TransactionMint was the initial idea, but then it breaks a lot of things and significantly makes transaction response usage worst because of small number of common properties.

then I did a refactor on typing stuff for Transaction which now(optionally) accepts a generic informing transaction type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@QuinnLee you can check the changed tests/files to understand better how it's working now

@LuizAsFight LuizAsFight enabled auto-merge (squash) November 17, 2022 21:30
@LuizAsFight LuizAsFight merged commit 90dc675 into master Nov 17, 2022
@LuizAsFight LuizAsFight deleted the lf-602/fix/transaction-decode-input branch November 17, 2022 21:54
@LuizAsFight LuizAsFight self-assigned this Sep 27, 2023
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.

Add decode transaction of Mint type SDK failing on parsing transaction in decoding inputs part
4 participants