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(tokenMintTransaction): Implement TokenMintTransaction E2E tests: TCK #310

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

rwalworth
Copy link
Contributor

Description:
This PR implements the TokenMintTransaction tests documented in docs/test-specifications/token-service/tokenMintTransaction.md.

Related issue(s):
#304

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
@rwalworth rwalworth added the enhancement New feature or request label Dec 12, 2024
@rwalworth rwalworth self-assigned this Dec 12, 2024
@rwalworth rwalworth linked an issue Dec 12, 2024 that may be closed by this pull request
rwalworth and others added 4 commits December 13, 2024 11:29
Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
…for TS

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>
…action

Signed-off-by: ivaylogarnev-limechain <ivaylo.garnev@limechain.tech>
Comment on lines +192 to +202
expect(
(
await JSONRPCRequest(this, "mintToken", {
tokenId,
amount,
commonTransactionParams: {
signers: [supplyKey],
},
})
).newTotalSupply,
).to.equal(amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails in JS.

Could "expect" have been mistakenly used here? The response only returns the transaction status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec says that newTotalSupply should be returned as well, along with serialNumbers if NFTs were minted

Comment on lines +211 to +217
const response = await JSONRPCRequest(this, "mintToken", {
tokenId,
metadata: [metadata],
commonTransactionParams: {
signers: [supplyKey],
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails in JS.

The response only returns the transaction status. Did you mean to query the token instead to retrieve the information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Response should include newTotalSupply and optionally serialNumbers

assert.fail("Should throw an error");
});

it("(#10) Mints a paused token", async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails with TOKEN_HAS_NO_SUPPLY_KEY instead of TOKEN_IS_PAUSED.

Shouldn't the tokenId be set in the mintToken transaction body rather than creating a new token that isn’t paused? When I do this, it succeeds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this is a typo, fixed.

Comment on lines +389 to +399
expect(
(
await JSONRPCRequest(this, "mintToken", {
tokenId,
amount,
commonTransactionParams: {
signers: [supplyKey],
},
})
).newTotalSupply,
).to.equal(amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails in JS.

Could "expect" have been mistakenly used here? The response only returns the transaction status

This is also valid for tests #2/#3/#4/#5/#9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Response should include newTotalSupply and optionally serialNumbers

});

describe("Metadata", function () {
it("(#1) Mints an NFT", async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails in JS.

The response only returns the transaction status

This is also valid for tests #1/#2/#4/#9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Response should include newTotalSupply and optionally serialNumbers

assert.fail("Should throw an error");
});

it("(#6) Mints an amount of 3 NFTs with 1 max supply", async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails in JS, it does not throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm it's passing for C++, are you creating the token with all the correct fields?

Signed-off-by: Rob Walworth <robert.walworth@swirldslabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TCK: Implement tests for TokenMintTransaction
2 participants