CCIP-2219 USDC Support - Stubbing interfaces and new flow #117

mateusz-sekara · 2024-09-12T11:31:25Z

Going top-down to fit USDC/CCTP support in ocr3

Please focus on the interfaces and connecting components together, implementation details are not that critical at this stage.

tokenData details are now part of the Observation step - we don't want to call any 3rd party API during the Outcome phase
TokenDataReader is replaced with TokenDataObserver, it's used during Observation. Goal here is to support multiple TokenDataObservers (per token) with one go
ExecuteOffchainConfig is enriched with the TokenDataObservers config under tokenDataObservers, please take a look at tests to understand the new schema

High-level config would be the following (skipping irrelevant details from the ExecuteOffchainConfig):

{
  "tokenDataObservers": [
    {
      "type": "usdc-cctp",
      "version": "1.0",
      "tokens": {
        "1": {
          "sourceTokenAddress": "0xabc",
          "sourceMessageTransmitterAddress": "0xefg"
        }
      },
      "attestationAPI": "http://localhost:8080",
      "attestationAPITimeout": "1s",
      "attestationAPIIntervalMilliseconds": "500ms"
    },
    {
      "type": "my-fancy-token",
      "version": "1.0",
      "tokens": {
        "2": {
          "sourceTokenAddress": "0xabc"
        }
      },
      "myFancyTokenAPI": "http://localhost:8080"
    }
  ]
}

Around 250 bytes per observer entry so it should be around 4k gas per processor (assuming 16 gas per byte and a single token per processor), it gives us roughly 4k entries to hit 12M gas (+ of course overhead of the existing config)

Changes in Chainlink repo smartcontractkit/chainlink#14418

winder · 2024-09-13T11:50:54Z

execute/exectypes/observation.go

+// TokenDataObservations are populated during the Observation phase and depends on previously fetched
+// MessageObservations details and the `tokenDataProcessors` configured in the ExecuteOffchainConfig.
+// Content of the MessageTokensData is determined by the TokenDataProcessor implementations.
+//   - if Message doesn't have any tokens, TokenData slice will be empty.


Suppose ChainSelector chain and SeqNum seq is a message that does not have token data. Is there no MessageTokenData object in the map? In that case tokenObs[chain][seq].IsReady() is false even though we're not waiting for data.

MessageTokenData has to be always there, that's gonna be a requirement here. Therefore client doesn't have to handle nils or other weird edge cases. We move this responsibility to the processor to init everything properly, even messages that don't have tokens. In this case, it should be populated with an empty MessageTokensData which is always IsReady=true in that case

func (mtd MessageTokensData) IsReady() bool { for _, td := range mtd.TokenData { if !td.IsReady() { return false } } return true }

I think comment explains that, no? TokenData slice will be empty and for empty TokenData we return IsReady=true

execute/tokendata/processor.go

makramkd

Overall looks good, just have a few suggestions and some comments on how versioning will be handled in the future will be useful. Also, since this breaks the build (due to a constructor signature change) lets have a PR in chainlink ready to do any fixes and make sure the integration test passes.

execute/exectypes/commit_data.go

execute/exectypes/observation.go

pluginconfig/execute.go

mateusz-sekara · 2024-09-16T10:10:52Z

Overall looks good, just have a few suggestions and some comments on how versioning will be handled in the future will be useful. Also, since this breaks the build (due to a constructor signature change) lets have a PR in chainlink ready to do any fixes and make sure the integration test passes.

@makramkd PR with fixes in the chainlink repo is linked in the description of that PR :P

execute/exectypes/observation.go

winder · 2024-09-16T12:27:20Z

execute/plugin.go

@@ -413,13 +423,13 @@ func (p *Plugin) Outcome(
 			context.Background(),
 			p.lggr,
 			p.msgHasher,
-			p.tokenDataReader,
 			p.reportCodec,
 			p.estimateProvider,
 			observation.Nonces,


Maybe Nonces should be added to selectReport instead of being part of the this constructor.

execute/plugin_functions.go

winder · 2024-09-16T12:34:05Z

execute/report/report.go

@@ -35,6 +34,11 @@ func buildSingleChainReportHelper(
 			readyMessages = make(map[int]struct{})
 		}
 		for i := 0; i < len(report.Messages); i++ {
+			if !report.MessageTokenData[i].IsReady() {


Move this below if len(report.MessageTokenData) != numMsg { to make sure there's no nil dereference

Actually, it's redundant, we check IsReady in the checkMessage function

winder · 2024-09-16T12:37:42Z

pluginconfig/execute.go

+	// Having version in that JSONisn't expensive, but it could reduce the risk of breaking the observers in the future.
+	Version string `json:"version"`
+
+	*USDCCCTPObserverConfig


No need for a pointer here, it's already a slice

A slice? Pointer is used here to better differentiate what is set and what not (based on the type), see the tests

Oops, I misread the code and thought it was defined as type USDCCCTPObserverConfig []TokenConfig.

I see what you mean, but you could do the same thing with. Now the dev doesn't have to worry about whether the pointer type is important

- if t.USDCCCTPObserverConfig == nil { + if (t.USDCCCTPObserverConfig == USDCCCTPObserverConfig{}) {

I can't make it work as you posted, probably because USDCCCTPObserverConfig contains a map and I can't just use == to compare that with empty state. It would require comparing all the fields or using reflect.DeepEqual()

IMO, a pointer is a more obvious choice if something is optional, isn't it?

Sure, you can keep the pointer

winder · 2024-09-16T12:38:17Z

pluginconfig/execute.go

+// Validate checks that the observer's config is semantically correct - fields are set correctly
+// depending on the config's type
+func (t TokenDataObserverConfig) Validate() error {


Any reason not to combine this with WellFormed?

I wanted to follow the existing pattern for the configs. Validate is a semantical check - verifying fields values. However, you can think of WellFormed as a syntactical check called during deserialization. We would not need that in an ideal world, because the JSON unmarshaller would cover it.

Assuming multiple embedded types in the future, the json Unmarshaller would still allow multiple of them to be initialized.

We only verify here if someone didn't create a JSON monster that contains all possible structs or something with mixed fields

Assuming multiple embedded types in the future, the json Unmarshaller would still allow multiple of them to be initialized.

We do have DecodeExecuteOffchainConfig(encodedExecuteOffchainConfig []byte) (ExecuteOffchainConfig, error) which is an entry point for deserialization and it's responsible for the JSON unmarshalling. This is also the place in which I call WellFormed() function. We can make this logic part of Validate but I feel it's better suited for DecodeExecuteOffchainConfig where we verify JSON against the expected schema. Therefore DecodeExecuteOffchainConfig always returns a syntactically valid object, meaning it would return an error if you try creating a single TokenDataObserverConfig that contains configs for both USDC and MyFancyToken

winder · 2024-09-16T12:40:14Z

pluginconfig/execute.go

+	AttestationAPIInterval *commonconfig.Duration      `json:"attestationAPIInterval"`
+}
+
+func (p USDCCCTPObserverConfig) Validate() error {


Check these as well?

Suggested change

func (p USDCCCTPObserverConfig) Validate() error {

func (p USDCCCTPObserverConfig) Validate() error {

if p.AttestationAPITimeout == nil {

return errors.New("AttestationAPITimeout not set")

}

if p.AttestationAPIInterval == nil {

return errors.New("AttestationAPIInterval not set")

}

I've made these to be values instead of pointers

Does 0 need to be an allowable value? IIRC these are hardcoded in the ocr2 plugin

The short answer, "I don't know" :D Long answer, it's gonna be part of the follow-up PR. We have hardcoded values in the v1 and I don't know how to handle defaults yet. Once I backport usdc logic it should become more obvious on how to handle that.

winder · 2024-09-16T12:52:09Z

pluginconfig/execute.go

+	SourceTokenAddress string `json:"sourceTokenAddress"`
+	// SourceMessageTransmitterAddr is the address of the CCTP MessageTransmitter address on the source chain
+	// https://github.com/circlefin/evm-cctp-contracts/blob/adb2a382b09ea574f4d18d8af5b6706e8ed9b8f2/src/MessageTransmitter.sol
+	SourceMessageTransmitterAddr string `json:"sourceMessageTransmitterAddress"`


General comment not specific to this PR - we really need a proper address type to abstract the different ways we need to encode addresses.

dimkouv · 2024-09-16T12:39:11Z

execute/exectypes/commit_data.go

 	// TokenData for each message.
-	TokenData [][][]byte `json:"-"`
+	MessageTokenData []MessageTokenData `json:"messageTokenData"`


Making sure that type CommitData struct { are not the data expected to be in a commit report.

Because e.g. this comment indicates the opposite.

// CommitObservations contain the commit plugin report data organized by the source chain selector. type CommitObservations map[cciptypes.ChainSelector][]CommitData

No, it's not, it's just the aggregate for all the information necessary to build an exec report. Maybe naming is not perfect, but I don't want to change that as part of my PR.

execute/exectypes/observation.go

dimkouv · 2024-09-16T12:43:13Z

execute/tokendata/processor.go

+	"github.com/smartcontractkit/chainlink-ccip/execute/exectypes"
+)
+
+type TokenDataObserver interface {


Would be nice to have some doc in the interface and maybe the method.

I will do as followup, I'm going top-down here, so I wanted to start with interfaces. I think it would clarify once I have an implementation ready, then I will add missing docs

execute/tokendata/processor.go

execute/plugin.go

dimkouv · 2024-09-16T12:47:18Z

execute/plugin.go

@@ -243,8 +244,12 @@ func (p *Plugin) Observation(
 			groupedCommits[report.SourceChain] = append(groupedCommits[report.SourceChain], report)
 		}

-		// TODO: Fire off messages for an attestation check service.
-		return exectypes.NewObservation(groupedCommits, messages, nil).Encode()
+		tkData, err1 := p.tokenDataObserver.Observe(ctx, messages)


Is it going to be synchronous?

e.g. If Circle's API is slow, it affects our observation duration?

It's gonna be implementation detail, from the plugin perspective we behave like it's synchronous, plugin shouldn't know that. Under the hood we will figure out background processing etc

execute/plugin.go

asoliman92 · 2024-09-16T13:29:13Z

execute/exectypes/observation.go

+	return true
+}
+
+func (mtd MessageTokenData) Error() error {


Maybe use error group? Might be helpful for debugging.

It's just a dummy implementation for now. Followup PR will cover implementation details, everything should clarify then ;)

…faces

pluginconfig/execute.go

Co-authored-by: Will Winder <wwinder.unh@gmail.com>

Co-authored-by: Makram <makramkd@users.noreply.github.com>

github-actions · 2024-09-16T14:02:54Z

Test Coverage

Branch	Coverage
cctp-interfaces	68.9%
main	68.8%

mateusz-sekara force-pushed the cctp-interfaces branch 3 times, most recently from 231b0cc to e074237 Compare September 12, 2024 12:51

mateusz-sekara changed the title ~~USDC configuration~~ USDC Support Stub Sep 13, 2024

mateusz-sekara added 2 commits September 13, 2024 12:04

USDC configuration & interfaces

02ff598

Stubbing code using new interfaces

06ed932

mateusz-sekara force-pushed the cctp-interfaces branch from e6cacbb to b3707aa Compare September 13, 2024 10:04

mateusz-sekara changed the title ~~USDC Support Stub~~ USDC Support - Stubbing interfaces and new flow Sep 13, 2024

mateusz-sekara force-pushed the cctp-interfaces branch 4 times, most recently from 588ef54 to 300d770 Compare September 13, 2024 10:39

mateusz-sekara marked this pull request as ready for review September 13, 2024 10:40

mateusz-sekara requested a review from a team as a code owner September 13, 2024 10:40

Minor adjustments + godoc

0e8b109

mateusz-sekara force-pushed the cctp-interfaces branch from 300d770 to 0e8b109 Compare September 13, 2024 10:43

mateusz-sekara changed the title ~~USDC Support - Stubbing interfaces and new flow~~ CCIP-2219 USDC Support - Stubbing interfaces and new flow Sep 13, 2024

winder reviewed Sep 13, 2024

View reviewed changes

Config improvements

24d10c7

mateusz-sekara force-pushed the cctp-interfaces branch from 491bb32 to 24d10c7 Compare September 16, 2024 08:57

mateusz-sekara requested a review from winder September 16, 2024 08:59

makramkd reviewed Sep 16, 2024

View reviewed changes

execute/tokendata/processor.go Outdated Show resolved Hide resolved

mateusz-sekara requested a review from asoliman92 September 16, 2024 09:22

Config improvements

8e79706

mateusz-sekara requested a review from jhweintraub September 16, 2024 09:35

makramkd reviewed Sep 16, 2024

View reviewed changes

Minor fixes

6172e77

mateusz-sekara force-pushed the cctp-interfaces branch 2 times, most recently from 383e6c8 to b4a8c27 Compare September 16, 2024 10:54

Post review fixes

d6e26ae

mateusz-sekara force-pushed the cctp-interfaces branch from b4a8c27 to d6e26ae Compare September 16, 2024 10:55

mateusz-sekara requested a review from makramkd September 16, 2024 10:59

Merge branch 'main' into cctp-interfaces

128b748

winder reviewed Sep 16, 2024

View reviewed changes

execute/exectypes/observation.go Show resolved Hide resolved

winder reviewed Sep 16, 2024

View reviewed changes

execute/plugin_functions.go Show resolved Hide resolved

winder reviewed Sep 16, 2024

View reviewed changes

dimkouv reviewed Sep 16, 2024

View reviewed changes

asoliman92 reviewed Sep 16, 2024

View reviewed changes

mateusz-sekara added 3 commits September 16, 2024 15:30

Post review fixes

6152a83

Merge remote-tracking branch 'origin/cctp-interfaces' into cctp-inter…

0a969c6

…faces

Post review fixes

cece9c6

makramkd approved these changes Sep 16, 2024

View reviewed changes

pluginconfig/execute.go Outdated Show resolved Hide resolved

mateusz-sekara and others added 2 commits September 16, 2024 15:58

Update execute/plugin_functions.go

bc1d78e

Co-authored-by: Will Winder <wwinder.unh@gmail.com>

Update pluginconfig/execute.go

236449c

Co-authored-by: Makram <makramkd@users.noreply.github.com>

mateusz-sekara requested review from dimkouv, asoliman92 and winder September 16, 2024 14:03

winder approved these changes Sep 16, 2024

View reviewed changes

mateusz-sekara merged commit 9186497 into main Sep 16, 2024
3 checks passed

mateusz-sekara deleted the cctp-interfaces branch November 8, 2024 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCIP-2219 USDC Support - Stubbing interfaces and new flow #117

CCIP-2219 USDC Support - Stubbing interfaces and new flow #117

mateusz-sekara commented Sep 12, 2024 •

edited

Loading

winder Sep 13, 2024

mateusz-sekara Sep 13, 2024

mateusz-sekara Sep 13, 2024

makramkd left a comment

mateusz-sekara commented Sep 16, 2024

winder Sep 16, 2024

winder Sep 16, 2024

mateusz-sekara Sep 16, 2024

winder Sep 16, 2024

mateusz-sekara Sep 16, 2024

winder Sep 16, 2024

mateusz-sekara Sep 16, 2024

winder Sep 16, 2024

winder Sep 16, 2024

mateusz-sekara Sep 16, 2024

winder Sep 16, 2024

mateusz-sekara Sep 16, 2024

mateusz-sekara Sep 16, 2024

winder Sep 16, 2024

mateusz-sekara Sep 16, 2024

winder Sep 16, 2024

mateusz-sekara Sep 16, 2024

winder Sep 16, 2024

dimkouv Sep 16, 2024

mateusz-sekara Sep 16, 2024

dimkouv Sep 16, 2024

mateusz-sekara Sep 16, 2024

dimkouv Sep 16, 2024

mateusz-sekara Sep 16, 2024 •

edited

Loading

asoliman92 Sep 16, 2024

mateusz-sekara Sep 16, 2024

github-actions bot commented Sep 16, 2024

-func (p USDCCCTPObserverConfig) Validate() error {
+func (p USDCCCTPObserverConfig) Validate() error {
+	if p.AttestationAPITimeout == nil {
+		return errors.New("AttestationAPITimeout not set")
+	}
+	if p.AttestationAPIInterval == nil {
+		return errors.New("AttestationAPIInterval not set")
+	}

CCIP-2219 USDC Support - Stubbing interfaces and new flow #117

CCIP-2219 USDC Support - Stubbing interfaces and new flow #117

Conversation

mateusz-sekara commented Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makramkd left a comment

Choose a reason for hiding this comment

mateusz-sekara commented Sep 16, 2024

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

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

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

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

mateusz-sekara Sep 16, 2024 • edited Loading

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 Sep 16, 2024

Test Coverage

mateusz-sekara commented Sep 12, 2024 •

edited

Loading

mateusz-sekara Sep 16, 2024 •

edited

Loading