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 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 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.

13 changes: 12 additions & 1 deletion pkg/reader/ccip.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,11 +367,22 @@ 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, tokenAmount := range msg.Message.TokenAmounts {
tokenAmount.DestExecData, err = r.extraDataCodec.DecodeTokenAmountDestExecData(
tokenAmount.DestExecData,
sourceChainSelector,
)
if err != nil {
return nil, fmt.Errorf("failed to decode token amount destExecData (%v): %w", tokenAmount.DestExecData, err)
}
msg.Message.TokenAmounts[i] = tokenAmount
}
msgs = append(msgs, msg.Message)
}

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

Choose a reason for hiding this comment

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

Ditto here

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 provide special treatment for DestExecData in token message depends on source chain
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

  • what it does
  • why it does it
    ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

See the thread, posted a message

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