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: added dynamic feeAmount parameter to fungible and non-fungible token create with custom fee method #700

Conversation

quiet-node
Copy link
Member

@quiet-node quiet-node commented Mar 6, 2024

Description:

  • Added dynamic feeAmount parameter to fungible and non-fungible token create with custom fee method for TokenCreateCustom contract which is served in the System Contract DApp
  • Fixed unit tests to adapt new feeAmount parameter
  • updated System Contract DApp UI to adapt new feeAmount parameter

Related issue(s):

Fixes #329

UI updates:

  • new fee amount field is added to FT and NFT token create tabs
Screenshot 2024-03-06 at 12 16 03 PM Screenshot 2024-03-06 at 12 18 49 PM

Notes for reviewer:

Checklist

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

Copy link

github-actions bot commented Mar 6, 2024

Test Results

242 tests  ±0   236 ✔️ +1   8m 0s ⏱️ +31s
  72 suites ±0       6 💤 ±0 
  14 files   ±0       0  - 1 

Results for commit c540ccd. ± Comparison against base commit 7275451.

♻️ This comment has been updated with latest results.

acuarica
acuarica previously approved these changes Mar 6, 2024
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.

lgtm

Comment on lines 138 to 139
} else if (withCustomFee && Number(feeTokenAmount) <= 0) {
sanitizeErr = 'Custom fee amount must be positive';
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if applicable, should we also validate if the number if too big?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice input! Though I think we can address it in another PR.

@@ -73,7 +75,8 @@ export const createHederaFungibleToken = async (
treasury: string,
inputKeys: ICommonKeyObject[],
msgValue: string,
feeTokenAddress?: string
feeTokenAddress?: string,
feeTokenAmount?: number
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit, should we use feeAmount to be consistent with the rest of the PR?

Copy link
Member Author

@quiet-node quiet-node Mar 6, 2024

Choose a reason for hiding this comment

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

Gotcha I don't know why I left it as feeTokenAmount! Good catch thanks! Pushed in the fix!

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.

lgtm

@quiet-node quiet-node merged commit 9a18ac0 into main Mar 7, 2024
23 checks passed
@quiet-node quiet-node deleted the 329-in-tokencreatecustom-contract-the-ihederatokenservicefixedfee-can-be-set-to-accept-dynamic-params branch March 7, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Tooling tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In TokenCreateCustom contract, the IHederaTokenService.FixedFee can be set to accept dynamic params
2 participants