Minting edge cases and additional testing #3226

paweljakubas · 2022-04-07T12:57:34Z

Edge cases addressed:

We now reject asset names that are too long.
We now reject asset quantities that are out of bounds.
We now accept empty asset names.
We now handle minting to foreign addresses.

Comments

Issue Number

…ing out to selection

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

- use 4-character indents. - avoid variable-length indentation. - limit line lengths to 80 characters where practical. - use only a single blank line between top-level definitions. https://input-output-hk.github.io/adrestia/code/Coding-Standards

It's easier to identify and reuse an existing constructor if the list is sorted into alphabetical order.

Rename `CreatedTransactionWithTooLongAssetName` to `AssetNameTooLong`. This error message implies that we've actually created a transaction. But if an asset name is too long, then it isn't possble for us to have created a transaction. Therefore, we can't say that we've created one. Additionally, if an asset name is too long, then it's too long in all contexts. Therefore, it's enough to say that "the asset name is too long": we don't actually need to state the context. We already use this style in other areas. For example: - AssetNotPresent - WalletAlreadyExists

Rename `CreatedTransactionWithIncorrectAssetQuantity` to `AssetQuantityOutOfBounds`. If an asset quantity is out of bounds, then it's impossible for us to say we have created a transaction. Therefore, it's enough to say "asset quantity out of bounds". The `AssetQuantityOutOfBounds` error name is consistent with other existing error names: - `AssetNameTooLong` - `AssetNotPresent` - `AssetQuantityOutOfBounds`

The upper limit is 32 bytes, but this does not necessarily correspond to 16 characters. According to the UTF-8 standard, Unicode code points can require between 1 and 4 bytes to encode.

Mention that the upper limit is equal to 2^63-1.

Emphasise that when an address is not specified, that minted assets will be returned to the wallet as change, and that change addresses will assigned automatically.

- Move this constant to `Primitive.Types.Tx`, so that it is located together with similar constants. - Redefine it in terms of `Int64` rather that `Int`, which may be platform specific. - Rename it to `txMintBurnMaxTokenQuantity`, to emphasise that it only relates to minting and burning.

Rename `AssetQuantityOutOfBounds` to `MintOrBurnAssetQuantityOutOfBounds`. The upper limit for a token quantity in the `mint` field of a transaction is actually different from the upper limit for a token quantity in an ordinary transaction output. So we use a more specific name to emphasise this difference.

This name was a bit confusing, because it could be interpreted as being "a token name of the maximum length", when in actuality it is "the maximum length of a token name".

This is consistant with other constants in the `Tx` module.

jonathanknowles · 2022-04-15T09:53:57Z

bors r+

3226: Minting edge cases and additional testing r=jonathanknowles a=paweljakubas  Edge cases addressed: - [x] We now reject asset names that are too long. - [x] We now reject asset quantities that are out of bounds. - [x] We now accept empty asset names. - [x] We now handle minting to foreign addresses. ### Comments  ### Issue Number ADP-1542  Co-authored-by: Pawel Jakubas <jakubas.pawel@gmail.com> Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

piotr-iohk · 2022-04-15T12:25:08Z

Just running e2e tests against this branch (5f54412) -> https://github.com/input-output-hk/cardano-wallet/runs/6037549759?check_suite_focus=true.

And it seems I now get this:

{"code":"missing_policy_public_key",
 "message":"It seems the wallet lacks a policy public key. It's therefore not possible to create a minting or burning transaction. Please POST to endpoint /wallets/{walletId}/policy-key to set a policy key."}

even when I construct tx that does not include mint_burn field, which was not observed before (e.g. it was not observed when testing against da85cf4 from this branch).

piotr-iohk · 2022-04-15T12:25:21Z

bors r-

iohk-bors · 2022-04-15T12:25:23Z

Canceled.

jonathanknowles · 2022-04-15T13:01:54Z

Just running e2e tests against this branch

Hi @piotr-iohk.

Thanks for posting!

Just want to ask your opinion: if we think it's a requirement that the end-to-end test suite passes before a PR is merged, then what would you think about us incorporating the end-to-end test suite into CI?

Otherwise, if some essential test is not included in CI, I think it becomes trickier to reason about whether a PR should be merged or not. I would hope that we can make CI our single source of truth for whether a PR should be merged or not. (I definitely wouldn't have expected a merge to be cancelled for a test failure that's not part of CI, so this cancellation caught me by surprise! I hope we can figure out a way to avoid this kind of scenario. 😄 )

Of course, as you point out, if it is genuinely broken, then we definitely should add an integration test to cover this failure.

Thanks for your help!

paweljakubas · 2022-04-15T13:26:38Z

@jonathanknowles @piotr-iohk I will push a fix for this very soon

paweljakubas · 2022-04-15T13:41:54Z

bors r+

piotr-iohk · 2022-04-15T14:32:38Z

Just want to ask your opinion: if we think it's a requirement that the end-to-end test suite passes before a PR is merged, then what would you think about us incorporating the end-to-end test suite into CI?

Maybe that's something to discuss. From the top of my head I see two issues:

exec time (~1h for full e2e suite)
parallelism (they are not really designed to be run in parallel, so it may be a problem in case of concurrent PR merges)

On the other hand full e2e suite is run nightly so potential issues should be caught the next day. In practice those issues should be only those that cannot be caught with integration tests so it, I guess, it should be ok.

iohk-bors · 2022-04-15T14:39:47Z

Build succeeded:

3236: Adjust e2e tests for minting/burning edge cases r=piotr-iohk a=piotr-iohk  - [x] Adjust e2e tests for minting/burning edge cases ### Comments Adjust tests after #3226. ### Issue Number adp-1542 Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>

paweljakubas self-assigned this Apr 7, 2022

paweljakubas force-pushed the paweljakubas/adp-1542/minting-edge-cases branch from 423348f to 52b2a1b Compare April 8, 2022 07:48

paweljakubas changed the title ~~asset name too long~~ Minting edge cases and additional testing Apr 8, 2022

paweljakubas added 5 commits April 14, 2022 12:45

asset name too long

db7c9a3

wrong token quantity

088d96a

introduce optional token name with test

17c8066

add preliminary integration test for minting into foreign address

dd9d94a

introduce optional address when minting and add first version of mint…

fa51158

…ing out to selection

paweljakubas force-pushed the paweljakubas/adp-1542/minting-edge-cases branch from a031620 to fa51158 Compare April 14, 2022 12:43

paweljakubas added 2 commits April 14, 2022 16:48

update integration test

c19b17e

add token map coalescence functionality within the same address

b22eb75

paweljakubas requested review from jonathanknowles and piotr-iohk April 14, 2022 15:13

paweljakubas marked this pull request as ready for review April 14, 2022 15:13

paweljakubas commented Apr 14, 2022

View reviewed changes

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs Outdated Show resolved Hide resolved

paweljakubas and others added 3 commits April 14, 2022 17:58

hlint and stylish

56bc407

add fix for minUTxO value due to TxOuts with assets

480af69

jonathanknowles force-pushed the paweljakubas/adp-1542/minting-edge-cases branch from a66f71e to e038cbc Compare April 15, 2022 04:45

jonathanknowles added 4 commits April 15, 2022 05:43

Sort ApiErrorCode constructors into alphabetical order.

53f0228

It's easier to identify and reuse an existing constructor if the list is sorted into alphabetical order.

Revise error string for ErrConstructTxAssetNameTooLong.

bbe4ab6

The upper limit is 32 bytes, but this does not necessarily correspond to 16 characters. According to the UTF-8 standard, Unicode code points can require between 1 and 4 bytes to encode.

jonathanknowles force-pushed the paweljakubas/adp-1542/minting-edge-cases branch from 1f382d1 to 3846b54 Compare April 15, 2022 06:56

Revise error string for ErrConstructTxAssetQuantityOutOfBounds.

da85cf4

Mention that the upper limit is equal to 2^63-1.

jonathanknowles force-pushed the paweljakubas/adp-1542/minting-edge-cases branch from 3846b54 to da85cf4 Compare April 15, 2022 06:57

jonathanknowles added 4 commits April 15, 2022 07:05

Revise description of receivingAddress in ApiMintData.

4683073

Emphasise that when an address is not specified, that minted assets will be returned to the wallet as change, and that change addresses will assigned automatically.

Rename maxLengthTokenName to tokenNameMaxLength.

b75e153

This name was a bit confusing, because it could be interpreted as being "a token name of the maximum length", when in actuality it is "the maximum length of a token name".

Change type of txMintBurnMaxTokenQuantity to TokenQuantity.

5f54412

This is consistant with other constants in the `Tx` module.

jonathanknowles approved these changes Apr 15, 2022

View reviewed changes

piotr-iohk mentioned this pull request Apr 15, 2022

Adjust e2e tests for minting/burning edge cases #3236

Merged

1 task

treat policyXPub in more conservative way

646d2cb

piotr-iohk approved these changes Apr 15, 2022

View reviewed changes

iohk-bors bot merged commit 72d6eb7 into master Apr 15, 2022

iohk-bors bot deleted the paweljakubas/adp-1542/minting-edge-cases branch April 15, 2022 14:39

WilliamKingNoel-Bot pushed a commit that referenced this pull request Apr 15, 2022

docs: Merge #3226

6e607b4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minting edge cases and additional testing #3226

Minting edge cases and additional testing #3226

paweljakubas commented Apr 7, 2022 •

edited by jonathanknowles

Loading

jonathanknowles commented Apr 15, 2022

piotr-iohk commented Apr 15, 2022

piotr-iohk commented Apr 15, 2022

iohk-bors bot commented Apr 15, 2022

jonathanknowles commented Apr 15, 2022 •

edited

Loading

paweljakubas commented Apr 15, 2022

paweljakubas commented Apr 15, 2022

piotr-iohk commented Apr 15, 2022

iohk-bors bot commented Apr 15, 2022

Minting edge cases and additional testing #3226

Minting edge cases and additional testing #3226

Conversation

paweljakubas commented Apr 7, 2022 • edited by jonathanknowles Loading

Comments

Issue Number

jonathanknowles commented Apr 15, 2022

piotr-iohk commented Apr 15, 2022

piotr-iohk commented Apr 15, 2022

iohk-bors bot commented Apr 15, 2022

jonathanknowles commented Apr 15, 2022 • edited Loading

paweljakubas commented Apr 15, 2022

paweljakubas commented Apr 15, 2022

piotr-iohk commented Apr 15, 2022

iohk-bors bot commented Apr 15, 2022

paweljakubas commented Apr 7, 2022 •

edited by jonathanknowles

Loading

jonathanknowles commented Apr 15, 2022 •

edited

Loading