Skip to content
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

support for decoding destexecdata #472

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 75 additions & 16 deletions mocks/pkg/types/ccipocr3/extra_data_codec.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 9 additions & 1 deletion pkg/reader/ccip.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,11 +367,19 @@ func (r *ccipChainReader) MsgsBetweenSeqNums(
return nil, fmt.Errorf("failed to cast %v to Message", item.Data)
}

msg.Message.ExtraArgsDecoded, err = r.extraDataCodec.DecodeExtraData(msg.Message.ExtraArgs, sourceChainSelector)
msg.Message.ExtraArgsDecoded, err = r.extraDataCodec.DecodeExtraArgs(msg.Message.ExtraArgs, sourceChainSelector)
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for i, token := range msg.Message.TokenAmounts {
for i, tokenAmount := 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
msg.Message.TokenAmounts[i] = token
}
msgs = append(msgs, msg.Message)
}

Expand Down
3 changes: 2 additions & 1 deletion pkg/types/ccipocr3/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

DecodeTokenAmountDestExecData(destExecData Bytes, sourceChainSelector ChainSelector) (Bytes, error)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment here on what this is

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

}

// RMNCrypto provides a chain-agnostic interface for verifying RMN signatures.
Expand Down
Loading