-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Implement extra args codec #16016
base: solana-offchain-plugin
Are you sure you want to change the base?
Implement extra args codec #16016
Conversation
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
AER Report: CI Coreaer_workflow , commit , Clean Go Tidy & Generate , Detect Changes , Scheduled Run Frequency , GolangCI Lint (core/scripts) , Core Tests (go_core_tests) , GolangCI Lint (.) , Core Tests (go_core_tests_integration) , GolangCI Lint (integration-tests/load) , Core Tests (go_core_ccip_deployment_tests) , GolangCI Lint (integration-tests) , Core Tests (go_core_fuzz) , GolangCI Lint (deployment) , test-scripts , Core Tests (go_core_race_tests) , lint , SonarQube Scan 1. Too Many Requests - 429:[Setup NodeJS]Source of Error:
Why: The error indicates that the npm registry is throttling requests due to too many requests being made in a short period, resulting in a 429 status code. Suggested fix: Implement retry logic with exponential backoff or use a caching mechanism to reduce the number of requests made to the npm registry. 2. Improperly encoded boolean value:[Run tests]Source of Error:
Why: The test is failing because the ABI (Application Binary Interface) is not correctly encoding a boolean value, which is causing the decode function to throw an error. Suggested fix: Ensure that the boolean values are correctly encoded in the ABI before decoding. Review the encoding logic and correct any discrepancies. AER Report: Operator UI CI ran successfully ✅ |
@@ -43,6 +50,61 @@ func decodeExtraArgsV1V2(extraArgs []byte) (gasLimit *big.Int, err error) { | |||
return ifaces[0].(*big.Int), nil | |||
} | |||
|
|||
func decodeExtraArgsSVMV1(extraArgs []byte) (*message_hasher.ClientSVMExtraArgsV1, 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.
This is a method to decode from an EVM source chain right?
EVMExtraArgsKey: gas, | ||
}, nil | ||
|
||
case chainsel.FamilySolana: |
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 switch on family here is on the source chain selector, meaning this case applies when source was Solana chain.
Thus decoding this requires Solana decoder.
But the implementation is using EVM decoder.
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
case chainsel.FamilyEVM: | ||
gas, err1 := decodeExtraArgsV1V2(extraArgs) | ||
if err1 != nil { | ||
return nil, fmt.Errorf("failed to decode EVM extra data, %w", err) | ||
} | ||
|
||
return map[string]any{ | ||
EVMExtraArgsKey: gas, | ||
}, nil |
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'd be tempted to do some minor refactoring here so you can write it like this:
case chainsel.FamilyEVM: | |
gas, err1 := decodeExtraArgsV1V2(extraArgs) | |
if err1 != nil { | |
return nil, fmt.Errorf("failed to decode EVM extra data, %w", err) | |
} | |
return map[string]any{ | |
EVMExtraArgsKey: gas, | |
}, nil | |
case chainsel.FamilyEVM: | |
return decodeExtraArgsV1V2(extraArgs) |
Maybe even some moderate refactoring to avoid having SVM code in the EVM package.
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.
decodeExtraArgsV1V2
returns *big.int
. The return type needs to be map[string]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.
The way I understand, the SVMExtraArgsv1 is defined for EVM onchain, and requires abi decoding, which is all EVM
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.
Yes, I said there would be minor refactoring.
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
solana.SystemProgramID, | ||
}, | ||
extraArgs := ccip_router.AnyExtraArgs{ | ||
// TODO wait for onchain team fix this AnyExtraArgs |
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.
Since the source chain here is SVM, we won't need these SVM specific fields here, since you will not go from SVM -> SVM.
Atleast not until CCIP supports another SVM chain apart from Solana.
For testing right now, we should be good with just EVM specific fields in this struct.
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
t.Run("decode extra args into map svm", func(t *testing.T) { | ||
key, err := solana.NewRandomPrivateKey() | ||
require.NoError(t, err) | ||
encoded, err := d.contract.EncodeSVMExtraArgsV1(nil, message_hasher.ClientSVMExtraArgsV1{ |
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.
@prashantkumar1982 This is the failing test I was talking about. By inspecting the encoded bytes, I suspect this EncodeSVMExtraArgsV1
is not working properly.
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.
Could you also use d.contract.DecodeSVMExtraArgsV1(), and see if that gives you back the same fields as what you encoded?
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.
Yep, just updated the test. Used the d.contract.DecodeSVMExtraArgsV1
constructor to create the payload, but it seems to be the same thing. When I try encoding, the encoded bytes cannot be unpacked correctly either.
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
require.NoError(t, err) | ||
output, err := DecodeExtraArgsToMap(buf.Bytes()) | ||
require.NoError(t, err) | ||
require.Len(t, output, 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.
Can you add require.Equal
checks on the contents of the output
map?
func (ExtraArgsCodec) DecodeExtraData(extraArgs cciptypes.Bytes, sourceChainSelector cciptypes.ChainSelector) (map[string]any, error) { | ||
family, err := chainsel.GetSelectorFamily(uint64(sourceChainSelector)) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to decode extra data, %w", err) |
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.
return nil, fmt.Errorf("failed to decode extra data, %w", err) | |
return nil, fmt.Errorf("failed to get chain family for selector %d: %w", sourceChainSelector, err) |
"github.com/smartcontractkit/chainlink-ccip/chains/solana/gobindings/ccip_router" | ||
) | ||
|
||
func DecodeExtraArgsToMap(extraArgs []byte) (map[string]any, 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.
Good rule of thumb to add a docstring to exported methods
|
||
m, err := DecodeExtraArgsToMap(encoded) | ||
require.NoError(t, err) | ||
require.Len(t, m, 1) |
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.
Lets add require.Equal
checks on the contents of the map.
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 check for contents is already in the below 3 lines (53-55).
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 comparison is between a map, vs fields of a struct, so require.Equals will likely not work
|
||
m, err := DecodeExtraArgsToMap(encoded) | ||
require.NoError(t, err) | ||
require.Len(t, m, 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.
Ditto here re: equality checks.
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
|
||
m, err := DecodeExtraArgsToMap(encoded) | ||
require.NoError(t, err) | ||
require.Len(t, m, 1) |
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 check for contents is already in the below 3 lines (53-55).
|
||
m, err := DecodeExtraArgsToMap(encoded) | ||
require.NoError(t, err) | ||
require.Len(t, m, 1) |
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 comparison is between a map, vs fields of a struct, so require.Equals will likely not work
require.NoError(t, err) | ||
|
||
_, err = DecodeExtraArgsToMap(encoded) | ||
require.NoError(t, err) |
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.
once the test passes, we should verify the decoded data is same as originally passed in data.
t.Run("decode extra args into map svm", func(t *testing.T) { | ||
key, err := solana.NewRandomPrivateKey() | ||
require.NoError(t, err) | ||
encoded, err := d.contract.EncodeSVMExtraArgsV1(nil, message_hasher.ClientSVMExtraArgsV1{ |
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.
Could you also use d.contract.DecodeSVMExtraArgsV1(), and see if that gives you back the same fields as what you encoded?
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Quality Gate passedIssues Measures |
Jira
extra args codec for OCR message. Convert []byte into map[string]any format depends on the source chain.