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.

Sign up for GitHub

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

Jump to bottom

fix: returning non-success response code instead of reverting when supplykey is missing (#167) #223

Merged
merged 10 commits into from
Feb 13, 2025
Merged

fix: returning non-success response code instead of reverting when supplykey is missing (#167) #223

merged 10 commits into from
Feb 13, 2025

Conversation

arianejasuwienas
Copy link
Contributor

Description:

Mint and burn methods can not be executed when no supply key is set. But to make it work the same as on the mainnet we will return the response code 180 (HederaResponseCodes.TOKEN_HAS_NO_SUPPLY_KEY) then instead of reverting the transaction.

Related issue(s):

Fixes #167

Notes for reviewer:

Checklist

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

@arianejasuwienas arianejasuwienas self-assigned this Jan 29, 2025
@arianejasuwienas arianejasuwienas added the bug A error that causes the feature to behave differently than what was expected based on design docs label Jan 29, 2025
@arianejasuwienas arianejasuwienas marked this pull request as ready for review January 29, 2025 13:09
@arianejasuwienas arianejasuwienas requested a review from a team as a code owner January 29, 2025 13:09
@arianejasuwienas arianejasuwienas changed the title fix: returning code 180 instead of reverting when supplykey is missing fix: returning code 180 instead of reverting when supplykey is missing (#167) Jan 30, 2025
@arianejasuwienas arianejasuwienas changed the title fix: returning code 180 instead of reverting when supplykey is missing (#167) fix: returning non-success response code instead of reverting when supplykey is missing (#167) Jan 30, 2025
acuarica
acuarica requested changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

Looking good.

One more thing, there is a test in hts.e2e.js currently skipped "should not mint because there is no supply key'". We should enable it with this change.

@arianejasuwienas arianejasuwienas force-pushed the 167-correct-response-for-no-supply-key branch from c7bea12 to 488c8e1 Compare February 6, 2025 14:12
@arianejasuwienas arianejasuwienas changed the base branch from main to 167-fetching-the-addresses-of-the-token-key-owners February 6, 2025 14:13
@arianejasuwienas arianejasuwienas force-pushed the 167-correct-response-for-no-supply-key branch from 488c8e1 to 6f86473 Compare February 7, 2025 10:57
@arianejasuwienas
feat: returning non-success response code instead of reverting when s…
aff16b6 
…upplykey is missing (#167)

Signed-off-by: Mariusz Jasuwienas <[email protected]>
@arianejasuwienas arianejasuwienas force-pushed the 167-correct-response-for-no-supply-key branch from 6f86473 to aff16b6 Compare February 7, 2025 11:03
@arianejasuwienas arianejasuwienas changed the base branch from 167-fetching-the-addresses-of-the-token-key-owners to main February 7, 2025 11:04
@arianejasuwienas
Copy link
Contributor Author

This is completly new approach to implement this task. That is why I've even removed all of the previous commits from the branch.

@arianejasuwienas arianejasuwienas marked this pull request as draft February 7, 2025 11:14
@arianejasuwienas
feat: returning non-success response code instead of reverting when s…
f6d89ab 
…upplykey is missing in hh (#167)

Signed-off-by: Mariusz Jasuwienas <[email protected]>
@arianejasuwienas
feat: returning non-success response code instead of reverting when s…
b38f55a 
…upplykey is missing in hh (#167)

Signed-off-by: Mariusz Jasuwienas <[email protected]>
@arianejasuwienas arianejasuwienas marked this pull request as ready for review February 7, 2025 13:01
@arianejasuwienas arianejasuwienas mentioned this pull request Feb 7, 2025
Closed
2 tasks
acuarica
acuarica requested changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

We need to include validation tests from now on to ensure the functionality we add or change matches real HTS.

@acuarica
Copy link
Contributor

That is why I've even removed all of the previous commits from the branch.

No need, better to keep commits for future reference so others can see the what has been done.

acuarica
acuarica requested changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

Also please enable should not mint because there is no supply key' in hts.e2e.js` test. I checked and the test now passes. After that we should be good to go.

@arianejasuwienas
feat: applying pr suggestion (#167)
9de080e 
Signed-off-by: Mariusz Jasuwienas <[email protected]>
acuarica
acuarica approved these changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@acuarica acuarica left a comment

Choose a reason for hiding this comment

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

Great stuff! Thanks for the effort!

@arianejasuwienas arianejasuwienas merged commit b737b00 into main Feb 13, 2025
13 checks passed
@arianejasuwienas arianejasuwienas deleted the 167-correct-response-for-no-supply-key branch February 13, 2025 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acuarica acuarica acuarica approved these changes

Assignees

@arianejasuwienas arianejasuwienas

Labels
bug A error that causes the feature to behave differently than what was expected based on design docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix mintToken/burnToken when there is no supply key
2 participants
@arianejasuwienas @acuarica