-
Notifications
You must be signed in to change notification settings - Fork 54
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
dapp-feat: TransferMultiTokens feature complete (#319) #384
dapp-feat: TransferMultiTokens feature complete (#319) #384
Conversation
…ngibleTokensPublic Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
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.
Nice.
Some suggestions
@@ -116,7 +116,8 @@ describe('TokenTransferContract test suite', () => { | |||
baseContract as unknown as Contract, | |||
hederaTokenAddress, | |||
[senderA, senderB], | |||
[3, 6, 9] | |||
[-18, 3, 6, 9], |
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.
Create a single array constant with an appropriate name for this since it's reused so often
@@ -95,7 +98,7 @@ export const transferFungibleTokens = async ( | |||
}); | |||
} | |||
if (!sanitizeErr) { | |||
amounts.some((amount) => { | |||
amounts.slice(1).some((amount) => { |
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.
Why are you taking only the 1st element?
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.
Oh the amounts.slice(1)
actually helps skipping the first element of the array in the loop. The reason is that the initial item in the amounts
array represents the totalInputAmount
multiplied by -1, ensuring that the sum of the amounts can equal 0. For instance, if a client inputs 2 amounts, 100 and 200, the amounts
array would be [-300, 100, 200]. This is why, when sanitizing the parameter, we only need to check on the amounts provided by the client.
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.
Okay, a short comment with this context on top of this line to help readers
inputType: 'text', | ||
inputPlaceholder: 'Token address...', | ||
inputSize: 'md', | ||
inputFocusBorderColor: '#A98DF4', |
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.
inputFocusBorderColor: '#A98DF4', | |
inputFocusBorderColor: HEDERA_BRANDING_COLORS...., |
inputPlaceholder: 'Token address...', | ||
inputSize: 'md', | ||
inputFocusBorderColor: '#A98DF4', | ||
inputClassname: 'w-full border-white/30', |
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.
'w-full border-white/30' should be a constant
senderAddress: { | ||
inputType: 'text', | ||
inputPlaceholder: 'Sender address...', | ||
inputSize: 'md', |
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.
'md' should be a constant
Signed-off-by: Logan Nguyen <[email protected]>
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.
LG, just note the comment here or in a follow up
…graph#384) * dapp-update: added gasLimit to transferTokensPublic and transferNonFungibleTokensPublic Signed-off-by: Logan Nguyen <[email protected]> * dapp-feat: finished TransferMultiTokens feature (hashgraph#319) Signed-off-by: Logan Nguyen <[email protected]> * dapp-update: replaced NON_FUNGIBLE with NFT for consistency Signed-off-by: Logan Nguyen <[email protected]> * dapp-update: added a reusable component for param fields Signed-off-by: Logan Nguyen <[email protected]> --------- Signed-off-by: Logan Nguyen <[email protected]> Signed-off-by: Mo Shaikjee <[email protected]>
Description: TransferMultiTokens feature complete
Related issue(s): #314
Fixes #319
** UI Demo **:
Screen.Recording.2023-09-11.at.1.35.31.PM.mov
Notes for reviewer:
Checklist