support for decoding destexecdata #472

huangzhen1997 · 2025-01-17T22:52:06Z

core ref: 663859cd548ca40384ca02ac576961b5b5e53fc5

Add new function to the extradatacodec interface for parsing message destexecdata. The destExecData is used when Any2EVM or SVM2Any messages for storing uint32 dest chain gas prices in bytes. Because ABI uses big-endian encoding while Borsch uses little-endian encoding.

Corresponding change for chainlink repo: smartcontractkit/chainlink#15944

prashantkumar1982 · 2025-01-18T00:38:47Z

pkg/types/ccipocr3/interfaces.go

@@ -21,7 +21,8 @@ type MessageHasher interface {
 }

 type ExtraDataCodec interface {
-	DecodeExtraData(ExtraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeExtraArgs(extraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)


nit: add comment on which exact field on the Message struct does this work on.

We need comments on this entire struct, please add doc comments so this is understandable to people who maintain this codebase

prashantkumar1982 · 2025-01-18T00:42:27Z

pkg/types/ccipocr3/interfaces.go

@@ -21,7 +21,8 @@ type MessageHasher interface {
 }

 type ExtraDataCodec interface {
-	DecodeExtraData(ExtraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeExtraArgs(extraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeDestExecData(destExecData Bytes, sourceChainSelector ChainSelector) (Bytes, error)


To what format should this function decode to?
The input is []byte, and output is []byte, so its confusing.

Also, since this is []byte, it means theoretically a family can encode even a struct here, even though today EVM and SVM just use uint32. So I think, to handle any generic struct, we likely need the format similar to the ExtraArgsDecoded.

All the fields here in the generic Any2Any Message ideally should be in a format that is chain agnostic. So we can't have these in either source/destination specific coding.

Lets confirm with CCIP team if they agree here, or have other suggestions.

@winder @makramkd What's your guys thoughts about this ?

Could you ask the onchain team? They confirmed the top level ExtraArgs is an object. Is this an object also?

At a minimum, I expect that this type should be any or map[string]any.

prashantkumar1982 · 2025-01-18T00:44:15Z

pkg/types/ccipocr3/interfaces.go

@@ -21,7 +21,8 @@ type MessageHasher interface {
 }

 type ExtraDataCodec interface {
-	DecodeExtraData(ExtraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeExtraArgs(extraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeDestExecData(destExecData Bytes, sourceChainSelector ChainSelector) (Bytes, error)


nit: suggestion: function name rename to decodeTokenAmountDestExecData

makramkd · 2025-01-21T10:22:03Z

The destExecData is used when Any2EVM or SVM2Any messages for storing uint32 dest chain gas prices in bytes.

Hey @huangzhen1997 this doesn't make sense to me, why would we do this in ccip? I'm not aware of this as a feature or functionality.

makramkd · 2025-01-21T10:23:03Z

pkg/types/ccipocr3/interfaces.go

@@ -21,7 +21,8 @@ type MessageHasher interface {
 }

 type ExtraDataCodec interface {
-	DecodeExtraData(ExtraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeExtraArgs(extraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)


We need comments on this entire struct, please add doc comments so this is understandable to people who maintain this codebase

makramkd · 2025-01-21T10:23:18Z

pkg/types/ccipocr3/interfaces.go

@@ -21,7 +21,8 @@ type MessageHasher interface {
 }

 type ExtraDataCodec interface {
-	DecodeExtraData(ExtraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeExtraArgs(extraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeTokenAmountDestExecData(destExecData Bytes, sourceChainSelector ChainSelector) (Bytes, error)


Please add a comment here on what this is

makramkd · 2025-01-21T10:28:13Z

pkg/types/ccipocr3/interfaces.go

@@ -21,7 +21,8 @@ type MessageHasher interface {
 }

 type ExtraDataCodec interface {
-	DecodeExtraData(ExtraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeExtraArgs(extraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	DecodeTokenAmountDestExecData(destExecData Bytes, sourceChainSelector ChainSelector) (Bytes, error)


Ah I see, I believe this is referring to : https://github.com/smartcontractkit/chainlink/blob/99860e4df73602187a926df4cc65c3919232d8f6/contracts/src/v0.8/ccip/libraries/Internal.sol#L230-L232

Lets make that very clear here that this is what this interface is doing.

makramkd · 2025-01-21T10:29:22Z

pkg/reader/ccip.go

+		for i, token := range msg.Message.TokenAmounts {
+			token.DestExecData, err = r.extraDataCodec.DecodeTokenAmountDestExecData(token.DestExecData, sourceChainSelector)
+			if err != nil {
+				return nil, fmt.Errorf("failed to decode DestExecData: %w", err)


Suggested change

return nil, fmt.Errorf("failed to decode DestExecData: %w", err)

return nil, fmt.Errorf("failed to decode token amount destExecData (%v): %w", tokenAmount, err)

makramkd · 2025-01-21T10:31:52Z

pkg/reader/ccip.go

 		if err != nil {
 			return nil, fmt.Errorf("failed to decode ExtraArgs: %w", err)
 		}
 		msg.Message.Header.OnRamp = onRampAddress
+
+		for i, token := range msg.Message.TokenAmounts {


Suggested change

for i, token := range msg.Message.TokenAmounts {

for i, tokenAmount := range msg.Message.TokenAmounts {

…ccip into destExecData

huangzhen1997 · 2025-01-21T23:20:02Z

Not sure how to fix the CI test, it's using a different Chainlink repo I provided in PR description.

makramkd · 2025-01-22T14:38:05Z

pkg/types/ccipocr3/interfaces.go

-	DecodeExtraData(ExtraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	// DecodeExtraArgs reformat bytes into a chain agnostic map[string]any representation for extra args
+	DecodeExtraArgs(extraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	// DecodeTokenAmountDestExecData provide special treatment for DestExecData in token message depends on source chain


This comment doesn't tell me much, can we improve it by saying:

what it does

why it does it
?

I'm waiting for feedback here so I will have more details to provide in comment

See the thread, posted a message

makramkd · 2025-01-22T14:38:39Z

pkg/types/ccipocr3/interfaces.go

@@ -21,7 +21,10 @@ type MessageHasher interface {
 }

 type ExtraDataCodec interface {
-	DecodeExtraData(ExtraArgs Bytes, sourceChainSelector ChainSelector) (map[string]any, error)
+	// DecodeExtraArgs reformat bytes into a chain agnostic map[string]any representation for extra args


github-actions · 2025-01-23T21:46:38Z

Metric	`destExecData`	`main`
Coverage	76.6%	76.5%

support for decoding destexecdata

b881118

huangzhen1997 requested review from winder and prashantkumar1982 January 17, 2025 22:52

huangzhen1997 requested a review from a team as a code owner January 17, 2025 22:52

huangzhen1997 requested review from asoliman92, makramkd, dimkouv, mateusz-sekara, 0xAustinWang and b-gopalswami January 17, 2025 22:52

huangzhen1997 marked this pull request as draft January 17, 2025 22:55

huangzhen1997 mentioned this pull request Jan 17, 2025

destexecdata decoder support + refactor smartcontractkit/chainlink#15978

Draft

prashantkumar1982 reviewed Jan 18, 2025

View reviewed changes

huangzhen1997 added 3 commits January 17, 2025 20:35

update

29f0e9c

Merge branch 'main' into destExecData

b7e1875

test

fe93c83

huangzhen1997 marked this pull request as ready for review January 21, 2025 04:10

huangzhen1997 added 2 commits January 20, 2025 22:12

test

c3c0e89

test

2a3f472

makramkd requested changes Jan 21, 2025

View reviewed changes

makramkd reviewed Jan 21, 2025

View reviewed changes

huangzhen1997 added 7 commits January 21, 2025 09:43

Merge branch 'main' into destExecData

4e7531b

test

e2aca57

merge

97b4623

Merge branch 'main' into destExecData

e57098e

test

918008a

Merge branch 'destExecData' of github.com:smartcontractkit/chainlink-…

89267c6

…ccip into destExecData

comment

7bccab7

lint

2ff6c11

makramkd reviewed Jan 22, 2025

View reviewed changes

huangzhen1997 added 4 commits January 22, 2025 15:27

Merge branch 'main' into destExecData

3a9779a

refactor data type

8ccc619

Merge branch 'main' into destExecData

9929168

revert

588905c

huangzhen1997 requested review from makramkd and prashantkumar1982 January 23, 2025 21:40

update comments

1651910

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for decoding destexecdata #472

support for decoding destexecdata #472

huangzhen1997 commented Jan 17, 2025 •

edited

Loading

prashantkumar1982 Jan 18, 2025

makramkd Jan 21, 2025

prashantkumar1982 Jan 18, 2025

huangzhen1997 Jan 21, 2025

winder Jan 21, 2025

prashantkumar1982 Jan 18, 2025

makramkd commented Jan 21, 2025

makramkd Jan 21, 2025

makramkd Jan 21, 2025

makramkd Jan 21, 2025

huangzhen1997 Jan 21, 2025

makramkd Jan 21, 2025

makramkd Jan 21, 2025

huangzhen1997 commented Jan 21, 2025

makramkd Jan 22, 2025

huangzhen1997 Jan 22, 2025

makramkd Jan 23, 2025

makramkd Jan 22, 2025

github-actions bot commented Jan 23, 2025

	return nil, fmt.Errorf("failed to decode DestExecData: %w", err)
	return nil, fmt.Errorf("failed to decode token amount destExecData (%v): %w", tokenAmount, err)

	for i, token := range msg.Message.TokenAmounts {
	for i, tokenAmount := range msg.Message.TokenAmounts {

support for decoding destexecdata #472

Are you sure you want to change the base?

support for decoding destexecdata #472

Conversation

huangzhen1997 commented Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makramkd commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huangzhen1997 commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 23, 2025

huangzhen1997 commented Jan 17, 2025 •

edited

Loading