-
Notifications
You must be signed in to change notification settings - Fork 295
feat: added custom tx builder for sol #6606
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: master
Are you sure you want to change the base?
Conversation
25166f8
to
56dee9c
Compare
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.
Some concerns here, moving this to slack
modules/bitgo/test/v2/unit/wallet.ts
Outdated
recipients: testRecipients, | ||
}); | ||
|
||
const txPrebuild = await tssSolWallet.prebuildTransaction({ |
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.
We should throw an error for this case?
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.
Yeah, good catch. Added an assertion check
const isValidBase64 = (str: string): boolean => { | ||
try { | ||
const decoded = Buffer.from(str, 'base64').toString('base64'); | ||
return decoded === str; | ||
} catch { | ||
return false; | ||
} | ||
}; | ||
|
||
// Check if the data is valid hex | ||
const isValidHex = (str: string): boolean => { | ||
return /^[0-9A-Fa-f]*$/.test(str) && str.length % 2 === 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.
We can move this utilities in a dedicated util file?
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.
moved
7f1cdfd
to
89da244
Compare
- added custom instruction builder for sol - use new intent type solInstruction to build the tx in WP TICKET: TMS-1218
89da244
to
2ad49f7
Compare
@claude review this pr |
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.
Pull Request Overview
This PR adds custom instruction builder functionality for Solana to support arbitrary instruction execution. It introduces a new intent type solInstruction
that allows users to build transactions with custom Solana instructions without being limited to predefined instruction types.
- Adds new
solInstruction
intent type and transaction handling logic across the SDK - Implements custom instruction builder and factory methods for processing arbitrary Solana instructions
- Updates type definitions and validation to support string-based instruction format for TSS compatibility
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
modules/sdk-core/src/bitgo/wallet/wallet.ts | Adds handling for solInstruction transaction type in prebuild flow |
modules/sdk-core/src/bitgo/wallet/iWallet.ts | Adds solInstructions option to prebuild transaction parameters |
modules/sdk-core/src/bitgo/wallet/BuildParams.ts | Adds solInstructions parameter to build params validation |
modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts | Adds solInstructions type definitions for TSS operations |
modules/sdk-core/src/bitgo/utils/mpcUtils.ts | Adds validation for solInstruction intent type |
modules/sdk-core/src/account-lib/baseCoin/enum.ts | Adds new SolanaCustomInstructions transaction type |
modules/sdk-coin-sol/src/sol.ts | Implements coin-specific intent field handling for Solana instructions |
modules/sdk-coin-sol/src/lib/utils.ts | Adds validation utilities for base64 and hex string formats |
modules/sdk-coin-sol/src/lib/transaction.ts | Updates transaction handling for custom instruction type |
modules/sdk-coin-sol/src/lib/solInstructionFactory.ts | Converts string-based instruction format to TransactionInstruction objects |
modules/sdk-coin-sol/src/lib/instructionParamsFactory.ts | Adds parsing logic for custom instructions |
modules/sdk-coin-sol/src/lib/iface.ts | Updates CustomInstruction interface to use string-based format |
modules/sdk-coin-sol/src/lib/customInstructionBuilder.ts | Refactors builder to accept string-based instruction format with validation |
modules/sdk-coin-sol/test/unit/transactionBuilder/customInstructionBuilder.ts | Updates tests to use new string-based instruction format |
modules/sdk-coin-sol/test/unit/solInstructionFactory.ts | Adds comprehensive tests for TSS format instruction handling |
modules/bitgo/test/v2/unit/wallet.ts | Adds test coverage for solInstruction transaction flow |
Comments suppressed due to low confidence (1)
modules/sdk-core/src/bitgo/utils/mpcUtils.ts:128
- The intent type 'customInstructions' is added to the validation list but the actual intent type used elsewhere is 'solInstruction'. This inconsistency could cause validation to fail.
!['acceleration', 'fillNonce', 'transferToken', 'tokenApproval', 'customInstructions'].includes(params.intentType)
* @returns True if the string is valid hex, false otherwise | ||
*/ | ||
export function isValidHex(str: string): boolean { | ||
return /^[0-9A-Fa-f]*$/.test(str) && str.length % 2 === 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.
The regex allows empty strings to be considered valid hex, but an empty string has length 0 which satisfies the even length condition. This could lead to unexpected behavior when processing empty data strings.
return /^[0-9A-Fa-f]*$/.test(str) && str.length % 2 === 0; | |
return str.length > 0 && /^[0-9A-Fa-f]*$/.test(str) && str.length % 2 === 0; |
Copilot uses AI. Check for mistakes.
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.
👍
|
||
// Convert string data to Buffer | ||
let dataBuffer: Buffer; | ||
|
||
if (isValidBase64(params.data)) { | ||
dataBuffer = Buffer.from(params.data, 'base64'); | ||
} else if (isValidHex(params.data)) { | ||
dataBuffer = Buffer.from(params.data, 'hex'); | ||
} else { | ||
// Fallback to UTF-8 | ||
dataBuffer = Buffer.from(params.data, 'utf8'); |
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.
The data conversion logic tries base64 first, then hex, then UTF-8. However, some valid hex strings might also be valid base64, leading to incorrect interpretation. Consider adding explicit format indicators or more strict validation.
// Convert string data to Buffer | |
let dataBuffer: Buffer; | |
if (isValidBase64(params.data)) { | |
dataBuffer = Buffer.from(params.data, 'base64'); | |
} else if (isValidHex(params.data)) { | |
dataBuffer = Buffer.from(params.data, 'hex'); | |
} else { | |
// Fallback to UTF-8 | |
dataBuffer = Buffer.from(params.data, 'utf8'); | |
assert(params.dataFormat === 'base64' || params.dataFormat === 'hex' || params.dataFormat === 'utf8', 'Missing or invalid dataFormat in custom instruction'); | |
// Convert string data to Buffer using explicit format | |
let dataBuffer: Buffer; | |
switch (params.dataFormat) { | |
case 'base64': | |
assert(isValidBase64(params.data), 'data is not valid base64'); | |
dataBuffer = Buffer.from(params.data, 'base64'); | |
break; | |
case 'hex': | |
assert(isValidHex(params.data), 'data is not valid hex'); | |
dataBuffer = Buffer.from(params.data, 'hex'); | |
break; | |
case 'utf8': | |
dataBuffer = Buffer.from(params.data, 'utf8'); | |
break; | |
default: | |
throw new Error('Unsupported dataFormat in custom instruction'); |
Copilot uses AI. Check for mistakes.
TICKET: TMS-1218