-
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
Changes from 23 commits
a757a6e
7253bba
0ec8119
82a162f
2c7bd05
e8bbe9a
36a5ad1
3fa671b
af29932
2597b48
a99bdcc
5fe01d2
c49c04b
7204973
a903310
80b3e3b
afcd808
6bc6b29
a5f84ac
d6af37b
f9489ae
cf28c84
67e0038
a20765b
ee10849
f2952a0
a1eacf6
0b15808
55b33e3
8d4d702
fc7f0f7
9939559
6e6441d
7e96d52
2e8de08
5c76148
5bcf509
8af2a50
aa9841d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import ( | |
"math/rand" | ||
"testing" | ||
|
||
"github.com/gagliardetto/solana-go" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/smartcontractkit/chainlink/v2/core/gethwrappers/ccip/generated/message_hasher" | ||
|
@@ -38,4 +39,79 @@ func Test_decodeExtraArgs(t *testing.T) { | |
|
||
require.Equal(t, gasLimit, decodedGasLimit) | ||
}) | ||
|
||
t.Run("decode extra args into map evm v1", func(t *testing.T) { | ||
encoded, err := d.contract.EncodeEVMExtraArgsV1(nil, message_hasher.ClientEVMExtraArgsV1{ | ||
GasLimit: gasLimit, | ||
}) | ||
require.NoError(t, err) | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Lets add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
|
||
gl, exist := m["gasLimit"] | ||
require.True(t, exist) | ||
require.Equal(t, gl, gasLimit) | ||
}) | ||
|
||
t.Run("decode extra args into map evm v2", func(t *testing.T) { | ||
encoded, err := d.contract.EncodeEVMExtraArgsV2(nil, message_hasher.ClientEVMExtraArgsV2{ | ||
GasLimit: gasLimit, | ||
AllowOutOfOrderExecution: true, | ||
}) | ||
require.NoError(t, err) | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Ditto here re: equality checks. |
||
|
||
gl, exist := m["gasLimit"] | ||
require.True(t, exist) | ||
require.Equal(t, gl, gasLimit) | ||
|
||
ooe, exist := m["allowOutOfOrderExecution"] | ||
require.True(t, exist) | ||
require.Equal(t, true, ooe) | ||
}) | ||
|
||
t.Run("decode extra args into map svm", func(t *testing.T) { | ||
key, err := solana.NewRandomPrivateKey() | ||
require.NoError(t, err) | ||
cu := uint32(10000) | ||
bitmap := uint64(4) | ||
ooe := false | ||
tokenReceiver := [32]byte(key.PublicKey().Bytes()) | ||
accounts := [][32]byte{[32]byte(key.PublicKey().Bytes())} | ||
decoded, err := d.contract.DecodeSVMExtraArgsV1(nil, cu, bitmap, ooe, tokenReceiver, accounts) | ||
if err != nil { | ||
return | ||
} | ||
encoded, err := d.contract.EncodeSVMExtraArgsV1(nil, decoded) | ||
require.NoError(t, err) | ||
|
||
m, err := DecodeExtraArgsToMap(encoded) | ||
require.NoError(t, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
require.Len(t, m, 5) | ||
|
||
cuDecoded, exist := m["computeUnits"] | ||
require.True(t, exist) | ||
require.Equal(t, cuDecoded, cu) | ||
|
||
bitmapDecoded, exist := m["accountIsWritableBitmap"] | ||
require.True(t, exist) | ||
require.Equal(t, bitmapDecoded, bitmap) | ||
|
||
ooeDecoded, exist := m["allowOutOfOrderExecution"] | ||
require.True(t, exist) | ||
require.Equal(t, ooeDecoded, ooe) | ||
|
||
tokenReceiverDecoded, exist := m["tokenReceiver"] | ||
require.True(t, exist) | ||
require.Equal(t, tokenReceiverDecoded, tokenReceiver) | ||
|
||
accountsDecoded, exist := m["accounts"] | ||
require.True(t, exist) | ||
require.Equal(t, accountsDecoded, accounts) | ||
}) | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we rename or move these functions related to decoding to appropriately named files? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
package ccipsolana | ||
|
||
import ( | ||
"fmt" | ||
"reflect" | ||
|
||
agbinary "github.com/gagliardetto/binary" | ||
|
||
"github.com/smartcontractkit/chainlink-ccip/chains/solana/gobindings/ccip_router" | ||
) | ||
|
||
// DecodeExtraArgsToMap is a helper function for converting Borsh encoded extra args bytes into map[string]any, which will be saved in ocr report.message.ExtraArgsDecoded | ||
func DecodeExtraArgsToMap(extraArgs []byte) (map[string]any, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good rule of thumb to add a docstring to exported methods There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously, the EVM version had this comment and a special offset. What happened to it?
|
||
outputMap := make(map[string]any) | ||
var args ccip_router.AnyExtraArgs | ||
decoder := agbinary.NewBorshDecoder(extraArgs) | ||
err := args.UnmarshalWithDecoder(decoder) | ||
if err != nil { | ||
return outputMap, fmt.Errorf("failed to decode extra args: %w", err) | ||
} | ||
|
||
val := reflect.ValueOf(args) | ||
typ := reflect.TypeOf(args) | ||
|
||
for i := 0; i < val.NumField(); i++ { | ||
field := typ.Field(i) | ||
fieldValue := val.Field(i).Interface() | ||
outputMap[field.Name] = fieldValue | ||
} | ||
|
||
return outputMap, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package ccipsolana | ||
|
||
import ( | ||
"bytes" | ||
"testing" | ||
|
||
agbinary "github.com/gagliardetto/binary" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/smartcontractkit/chainlink-ccip/chains/solana/gobindings/ccip_router" | ||
) | ||
|
||
func Test_decodeExtraArgs(t *testing.T) { | ||
t.Run("decode extra args into map svm", func(t *testing.T) { | ||
extraArgs := ccip_router.AnyExtraArgs{ | ||
GasLimit: agbinary.Uint128{Lo: 5000, Hi: 0}, | ||
AllowOutOfOrderExecution: false, | ||
} | ||
|
||
var buf bytes.Buffer | ||
encoder := agbinary.NewBorshEncoder(&buf) | ||
err := extraArgs.MarshalWithEncoder(encoder) | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Can you add |
||
|
||
gasLimit, exist := output["GasLimit"] | ||
require.True(t, exist) | ||
require.Equal(t, agbinary.Uint128{Lo: 5000, Hi: 0}, gasLimit) | ||
|
||
ooe, exist := output["AllowOutOfOrderExecution"] | ||
require.True(t, exist) | ||
require.Equal(t, false, ooe) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
package common | ||
|
||
import ( | ||
"fmt" | ||
|
||
chainsel "github.com/smartcontractkit/chain-selectors" | ||
|
||
"github.com/smartcontractkit/chainlink/v2/core/capabilities/ccip/ccipevm" | ||
"github.com/smartcontractkit/chainlink/v2/core/capabilities/ccip/ccipsolana" | ||
|
||
cciptypes "github.com/smartcontractkit/chainlink-ccip/pkg/types/ccipocr3" | ||
) | ||
|
||
type ExtraArgsCodec struct{} | ||
|
||
func NewExtraArgsCodec() ExtraArgsCodec { | ||
return ExtraArgsCodec{} | ||
} | ||
|
||
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 get chain family for selector %d: %w", sourceChainSelector, err) | ||
} | ||
|
||
switch family { | ||
case chainsel.FamilyEVM: | ||
return ccipevm.DecodeExtraArgsToMap(extraArgs) | ||
|
||
case chainsel.FamilySolana: | ||
return ccipsolana.DecodeExtraArgsToMap(extraArgs) | ||
|
||
default: | ||
return nil, fmt.Errorf("unsupported family for extra args type %s", family) | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice 👍 |
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.
nit: move these comments to the actual case block where you set extraByteOffset