-
Notifications
You must be signed in to change notification settings - Fork 20
Extra Args encoding/decoding #42
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: svm-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.
Pull Request Overview
This PR implements encoding/decoding functionality for CCIP extra args targeting both EVM (EVMExtraArgsV2
) and SVM (SVMExtraArgsV1
) destinations. The implementation provides type-safe encoding/decoding with proper serialization formats for each chain type.
- Adds EVM extra args V2 encoder/decoder using ABI encoding with gas limit and execution order configuration
- Adds SVM extra args V1 encoder/decoder using Borsh-like serialization for Solana-specific parameters
- Includes comprehensive test coverage for both implementations with round-trip validation
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/ccip-svm/src/index.ts | Exports the new encoder modules |
packages/ccip-svm/src/encoders/evmExtraArgsV2.ts | EVM extra args V2 encoding/decoding using ABI parameters |
packages/ccip-svm/src/encoders/svmExtraArgsV1.ts | SVM extra args V1 encoding/decoding using Borsh-like serialization |
packages/ccip-svm/src/tests/evmExtraArgsV2.test.ts | Comprehensive test suite for EVM extra args functionality |
packages/ccip-svm/src/tests/svmExtraArgsV1.test.ts | Comprehensive test suite for SVM extra args functionality |
packages/ccip-svm/package.json | Updates test script and adds vitest dependency |
Files not reviewed (2)
- packages/ccip-svm/package-lock.json: Language not supported
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
LGTM. Please consider the DevX angle /comments carefully before merging.
} | ||
|
||
/** | ||
* Creates properly encoded extraArgs buffer for SVM destinations |
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.
Consider linking to docs or otherwise putting in some details into how the extraArgs are computed? many users of the SDK will not fully understand the operations within the implementation of the function so would help them to know where to 'study up' on this?
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
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.
@andrejrakic to not be brittle for future versions should re link to one level higher in the docs tree? docs urls may change too
https://docs.chain.link/ccip/api-reference/svm instead of /v0.1.1 etc?
allowOutOfOrderExecution = true, | ||
tokenReceiver, | ||
accounts = [] | ||
}: SVMExtraArgsV1): Uint8Array { |
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.
Just had a thought: To make this encodeFunction easier to use (in evm and SVM) should it return hex rather than Uint8Array? In the tests for example we have convert the value of expected
into uint8Array
From a devx perspective will most people recognise hex more?
we could offer a utility (if needed by the SDK) to convert hex to Uint8Array but is there a better devx if encode/decode etc work with 0x-prefixed hexes to make it more familiar to users (including where uncaught exceptions arise etc?)
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.
Or can we provide a util function to convert to human readable
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.
Added hex utility functions to a new hex.ts
file and update both implementation and test files to leverage it. Here's how one can use it:
// Convert bytes to hex
const bytes = new Uint8Array([0x18, 0x1d, 0xcf, 0x10])
const hex = toHex(bytes) // "0x181dcf10"
// Convert hex to bytes
const bytes2 = fromHex("0x181dcf10") // Uint8Array([24, 29, 207, 16])
const bytes3 = fromHexBuffer("181dcf10") // Same result, using Buffer
…0% coverage. Add prettier and eslint
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
Copilot reviewed 12 out of 15 changed files in this pull request and generated 4 comments.
Files not reviewed (2)
- packages/ccip-svm/package-lock.json: Language not supported
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
const cleanHex = hex.startsWith('0x') ? hex.slice(2) : hex | ||
return new Uint8Array(Buffer.from(cleanHex, 'hex')) |
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 fromHexBuffer
function duplicates the hex cleaning logic from fromHex
. Consider consolidating this logic or having fromHexBuffer
call fromHex
to avoid code duplication.
const cleanHex = hex.startsWith('0x') ? hex.slice(2) : hex | |
return new Uint8Array(Buffer.from(cleanHex, 'hex')) | |
return fromHex(hex) |
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.
@andrejrakic please consider
|
||
// Serialize token_receiver ([u8; 32]) | ||
const encoder = getAddressEncoder() | ||
const tokenReceiverBytes = tokenReceiver ? encoder.encode(tokenReceiver) : new Uint8Array(32) // Default/zero 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.
The magic number 32
should be defined as a named constant (e.g., ADDRESS_SIZE = 32
) to improve code readability and maintainability.
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.
@andrejrakic +1 to this
* @returns Decoded EVMExtraArgsV2 object | ||
*/ | ||
export function decodeEVMExtraArgsV2(data: Uint8Array): EVMExtraArgsV2 { | ||
if (data.length < 68) { |
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 magic number 68
should be defined as a named constant (e.g., MIN_EVM_EXTRA_ARGS_V2_SIZE = 68
) to improve code readability and maintainability.
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.
@andrejrakic good nit here.
*/ | ||
export function decodeSVMExtraArgsV1(data: Uint8Array): SVMExtraArgsV1 { | ||
// 4 bytes tag + 4 bytes computeUnits + 8 bytes bitmap + 1 byte bool + 32 bytes tokenReceiver + 4 bytes accounts length | ||
if (data.length < 4 + 4 + 8 + 1 + 32 + 4) { |
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 magic numbers in the length calculation should be defined as named constants (e.g., TAG_SIZE = 4
, COMPUTE_UNITS_SIZE = 4
, etc.) to improve code readability and maintainability.
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.
+1 to this @andrejrakic
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.
LGTM - minor comments + copilot suggestions. Merge when ready!
describe('round-trip encoding/decoding', () => { | ||
it('should maintain data integrity through encode/decode cycle', () => { | ||
const testCases = [ | ||
{ gasLimit: 0n, allowOutOfOrderExecution: true }, |
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 you use table based testing -- consider adding a name property to each case, so that its ID'd in console.logs
allowOutOfOrderExecution = true, | ||
tokenReceiver, | ||
accounts = [] | ||
}: SVMExtraArgsV1): Uint8Array { |
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.
Or can we provide a util function to convert to human readable
* @returns Decoded EVMExtraArgsV2 object | ||
*/ | ||
export function decodeEVMExtraArgsV2(data: Uint8Array): EVMExtraArgsV2 { | ||
if (data.length < 68) { |
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.
@andrejrakic good nit here.
} | ||
|
||
/** | ||
* Creates properly encoded extraArgs buffer for SVM destinations |
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.
@andrejrakic to not be brittle for future versions should re link to one level higher in the docs tree? docs urls may change too
https://docs.chain.link/ccip/api-reference/svm instead of /v0.1.1 etc?
|
||
// Serialize token_receiver ([u8; 32]) | ||
const encoder = getAddressEncoder() | ||
const tokenReceiverBytes = tokenReceiver ? encoder.encode(tokenReceiver) : new Uint8Array(32) // Default/zero 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.
@andrejrakic +1 to this
*/ | ||
export function decodeSVMExtraArgsV1(data: Uint8Array): SVMExtraArgsV1 { | ||
// 4 bytes tag + 4 bytes computeUnits + 8 bytes bitmap + 1 byte bool + 32 bytes tokenReceiver + 4 bytes accounts length | ||
if (data.length < 4 + 4 + 8 + 1 + 32 + 4) { |
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.
+1 to this @andrejrakic
const cleanHex = hex.startsWith('0x') ? hex.slice(2) : hex | ||
return new Uint8Array(Buffer.from(cleanHex, 'hex')) |
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.
@andrejrakic please consider
This PR implements encoding/decoding functionality for
EVMExtraArgsV2
andSVMExtraArgsV1
. To test it, run: