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

[Token] Fix token cannot be burned. #5360

Closed
wants to merge 1 commit into from

Conversation

Stumble
Copy link
Contributor

@Stumble Stumble commented Oct 31, 2022

Description

Since #5096 is merged, this PR just add two test cases to cover the test path.
This commit also fixes a typo in testcase.

Test Plan

Two test cases have been added.


This change is Reviewable

@Stumble Stumble requested a review from areshand as a code owner October 31, 2022 00:39
@areshand
Copy link
Contributor

it is already fixed in #5096

@Stumble
Copy link
Contributor Author

Stumble commented Oct 31, 2022

@areshand awesome! Do you want to cherry-pick those two test cases and another fix for the typo into that PR?

1. Add two test cases to cover burning token of a limited supply in a
collection of unlimited supply.
2. Fix a typo in a test case that was not checking the right key.
@Stumble Stumble force-pushed the yumin/fix-token-burn branch from 54d3035 to e548039 Compare October 31, 2022 04:10
@Stumble
Copy link
Contributor Author

Stumble commented Oct 31, 2022

@areshand I've rebased the PR to just include the tests and typo fix.

@areshand
Copy link
Contributor

areshand commented Nov 4, 2022

Thanks for the PR. we currently don't want to merge test only changes

@areshand areshand closed this Nov 4, 2022
@Stumble
Copy link
Contributor Author

Stumble commented Nov 4, 2022

sure, np. Feel free to make this change when you guys merge commits, it fixes an incorrect test.

        assert!(!table::contains(&collections.collection_data, get_collection_name()), 1); // <--
        assert!(!table::contains(&collections.token_data, token_id.token_data_id), 1);

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.

2 participants