-
Notifications
You must be signed in to change notification settings - Fork 0
Feat: Add feeFactory #27
Feat: Add feeFactory #27
Conversation
Signed-off-by: Stanislaw Mazowiecki <[email protected]>
test/e2e/feeFactoryE2E.test.ts
Outdated
beforeEach(async () => { | ||
new HederaNFTSDK(myAccountId, myPrivateKey); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I'm creating collections in this test. For this, I need to be logged in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nftSDK
is already created with these keys in test/e2e/e2eConsts.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, sorry
if (hbarAmount) { | ||
fixedFee.setHbarAmount(Hbar.fromString(hbarAmount.toString())); | ||
} | ||
|
||
if (amount) { | ||
fixedFee.setAmount(amount); | ||
} | ||
|
||
if (denominatingTokenId) { | ||
fixedFee.setDenominatingTokenId(denominatingTokenId); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hbarAmount
, amount
and denominatingTokenId
are related to each other.
Either hbarAmount
is set and (amount
and denominatingTokenId
) are not set
OR
(amount
and denominatingTokenId
) are set and hbarAmount
is not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if (fallbackFee) { | ||
fixedFee = createFixedFeeFunction({ | ||
collectorAccountId: fallbackFee.collectorAccountId, | ||
hbarAmount: fallbackFee.hbarAmount, | ||
amount: fallbackFee.amount, | ||
denominatingTokenId: fallbackFee.denominatingTokenId, | ||
allCollectorsAreExempt: fallbackFee.allCollectorsAreExempt, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this was moved after the CustomRoyaltyFee creation this if and
if (fixedFee) {
royaltyFee.setFallbackFee(fixedFee);
}
could be merge into one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/types/validateProps.ts
Outdated
customFees?: CustomFee[]; | ||
}; | ||
|
||
export type validateFixedFeeFunction = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a function rename, this to something like: fixedFeeValidationProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/types/validateProps.ts
Outdated
collectorAccountId?: string; | ||
}; | ||
|
||
export type validateRoyaltyFeeFunction = validateFixedFeeFunction & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a function rename, this to something like: royaltyFeeValidationProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/utils/validateProps.ts
Outdated
validateCollectorAccountId(props); | ||
validateNumerator(props); | ||
validateDenominator(props); | ||
}; | ||
|
||
const validateNumerator = (props: validateRoyaltyFeeFunction) => { | ||
const validateHbarAmountAmountAndDenominatingToken = (props: fixedFeeValidationProps) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please write unit tests for all possible combinations
(false
- not set, true
- set)
hbarAmount | amount | denominatingTokenId | expectedResult |
---|---|---|---|
false | false | false | error |
false | false | true | error |
false | true | false | error |
false | true | true | valid |
true | false | false | valid |
true | false | true | error |
true | true | false | error |
true | true | true | error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/utils/validateProps.ts
Outdated
(props.hbarAmount && (props.amount || props.denominatingTokenId)) || | ||
(!props.hbarAmount && (!props.amount || !props.denominatingTokenId)) | ||
) { | ||
throw new Error(dictionary.createCollection.hbarAmountAmountAndDenominatingToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hbarAmountAmountAndDenominatingToken
can be renamed to hbarAmountOrAmountAndDenominatingToken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
…ollection # Conflicts: # src/HederaNFTSDK.ts # src/types/validateProps.ts # src/utils/validateProps.ts # test/e2e/createCollectionE2E.test.ts # test/e2e/e2eConsts.ts # test/unit/validateProps.test.ts
Description:
Related issue(s):
Fixes #25
Checklist