-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/new tokens node #49
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
base: main
Are you sure you want to change the base?
Conversation
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.
7 files reviewed, 7 comments
| { name: 'Ukrainian (Ukraine)', value: 'uk-UA' }, | ||
| { name: 'Thai (Thailand)', value: 'th-TH' }, | ||
| { name: 'Vietnamese (Vietnam)', value: 'vi-VN' }, | ||
| { name: 'Klingon', value: 'Klingon' }, |
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.
style: Klingon locale option seems out of place among standard language locales
| if (!name || !image || !description) { | ||
| throw new NodeOperationError(context.getNode(), 'Name, Image, and Description are required for metadata object mode', { | ||
| itemIndex, | ||
| }); | ||
| } |
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.
style: Consider using the validateRequiredField utility function consistently instead of manual validation to match the pattern used elsewhere in the function
| if (attributesJson && attributesJson.trim() !== '') { | ||
| try { | ||
| const attributes = JSON.parse(attributesJson); | ||
| if (Array.isArray(attributes)) { | ||
| metadata.attributes = attributes; | ||
| } |
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.
logic: Silent failure when attributes is not an array - consider adding a warning or error message to inform users
| export function buildTokenRecipient( | ||
| recipientData: WalletLocatorData, | ||
| chain?: string, | ||
| context?: any, | ||
| itemIndex?: number | ||
| ): string { | ||
| if (recipientData.mode === 'address') { | ||
| const address = recipientData.value; | ||
| if (!address || address.trim() === '') { | ||
| throw new NodeOperationError(context?.getNode(), 'Wallet address is required', { | ||
| itemIndex, | ||
| }); | ||
| } | ||
|
|
||
| if (address.startsWith('0x')) { | ||
| return `polygon:${address}`; | ||
| } else { | ||
| return `solana:${address}`; | ||
| } | ||
| } else { | ||
| const value = recipientData.value; | ||
| if (!value || value.trim() === '') { | ||
| throw new NodeOperationError(context?.getNode(), 'Recipient value is required', { | ||
| itemIndex, | ||
| }); | ||
| } | ||
| return `${recipientData.mode}:${value}:${chain}`; | ||
| } | ||
| } | ||
|
|
||
| export function buildTokenWalletIdentifier( | ||
| walletIdentifierData: WalletLocatorData, | ||
| chain?: string, | ||
| context?: any, | ||
| itemIndex?: number | ||
| ): string { | ||
| if (walletIdentifierData.mode === 'address') { | ||
| const address = walletIdentifierData.value; | ||
| if (!address || address.trim() === '') { | ||
| throw new NodeOperationError(context?.getNode(), 'Wallet address is required', { | ||
| itemIndex, | ||
| }); | ||
| } | ||
|
|
||
| if (address.startsWith('0x')) { | ||
| return `polygon:${address}`; | ||
| } else { | ||
| return `solana:${address}`; | ||
| } | ||
| } else { | ||
| const value = walletIdentifierData.value; | ||
| if (!value || value.trim() === '') { | ||
| throw new NodeOperationError(context?.getNode(), 'Wallet identifier value is required', { | ||
| itemIndex, | ||
| }); | ||
| } | ||
| return `${walletIdentifierData.mode}:${value}:${chain}`; | ||
| } | ||
| } |
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.
style: Both functions contain nearly identical logic. Consider extracting common functionality to reduce code duplication.
| if (address.startsWith('0x')) { | ||
| return `polygon:${address}`; | ||
| } else { | ||
| return `solana:${address}`; | ||
| } |
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.
logic: Address format detection using startsWith('0x') is fragile. Consider using a more robust blockchain detection method or explicit chain parameter.
| export function buildTokenRecipient( | ||
| recipientData: WalletLocatorData, | ||
| chain?: string, | ||
| context?: any, |
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.
style: Inconsistent typing: context?: any differs from existing functions that use proper type casting. Should match the pattern on line 18.
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.
5 files reviewed, 6 comments
| export interface TransferTokenRequest { | ||
| recipient: string; | ||
| amount: string; | ||
| amount?: string; |
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.
logic: Making amount optional is a breaking change. Ensure all consumers handle undefined amount appropriately to prevent runtime errors.
| console.log('API Request Failed:'); | ||
| console.log(JSON.stringify({ | ||
| endpoint, | ||
| requestBody, | ||
| apiVersion: API_VERSIONS.WALLETS, | ||
| }, null, 2)); |
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.
style: Debug console.log statements in production code - consider using proper logging or removing
| ): Promise<IDataObject> { | ||
| const tknChain = context.getNodeParameter('tknChain', itemIndex) as string; | ||
| const tknName = context.getNodeParameter('tknName', itemIndex) as string; | ||
| const blockchainType = context.getNodeParameter('blockchainType', itemIndex) as string; |
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.
logic: Missing validation for blockchainType parameter - other required parameters are validated but this one is not
| recipient: recipient, | ||
| }; | ||
|
|
||
|
|
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.
style: Extra blank line should be removed for consistency
| console.log('API Request Failed:'); | ||
| console.log(JSON.stringify({ | ||
| endpoint, | ||
| requestBody, | ||
| apiVersion: API_VERSIONS.WALLETS, | ||
| }, null, 2)); |
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.
style: Debug logging to console in production code - consider using proper logging mechanism or removing before production
| const responseParams = (rawResponse as IDataObject).params as { calls?: Array<{ chain?: string }> }; | ||
| if (responseParams && responseParams.calls && responseParams.calls[0]) { | ||
| chain = responseParams.calls[0].chain; | ||
| } |
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.
logic: Chain extraction logic uses unsafe array access - should check array length before accessing calls[0]
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.
I'm learning! Leave a 👍 or a 👎 to help me learn. Extra points if you leave a comment telling me why i'm wrong tagging @greptileai
1 file reviewed, no comments
No description provided.