Contract Discovery Processor.

[winder]

Implements a new discovery processor to observe contract addresses and bind the results.

[makramkd]

Why a pointer to an interface?

[winder]

We need to make sure that when the reader is updated here it is also updated in other parts of the plugin.

It works without the pointer because the shallow copies mean each slice refers to the same array, but will that always be the case?

[makramkd]

The implementation is a pointer receiver so it should work without the pointer. However, I kind of see what you're saying about the shared aspect of it.

The way the reader is used right now, its basically a singleton. The way things are designed right now, I don't see this not being the case. Feels like a change in this would cause a super subtle bug though, gotta think

[makramkd]

Correct me if I'm wrong on this but I think the type assertion on the ccipChainReader type helps us avoid this problem.

// Interface compliance check - on the pointer type
var _ CCIPReader = (*ccipChainReader)(nil)
// Interface compliance check - on the value type - FAILS
var _ CCIPReader = ccipChainReader{}

So as long as:

• CCIP reader is a singleton
• interface compliance is on the pointer receiver

It should be safe to have this as just the interface rather than pointer to interface. We can put a comment on the compliance check to ensure that this cannot change and what the implications are.

[makramkd]

Maybe there's a test we can write to ensure that the pointer type implements the interface - not sure though, seems out of scope of the Go testing facilities since its part of the type system.

[winder]

There are examples in the stdlib of tricks to avoid accidentally copying things that shouldn't be copied.

strings.Builder

// A Builder is used to efficiently build a string using [Builder.Write] methods.
// It minimizes memory copying. The zero value is ready to use.
// Do not copy a non-zero Builder.
type Builder struct {
	addr *Builder // of receiver, to detect copies by value
	buf  []byte
}

io.File

// File represents an open file descriptor.
type File struct {
	*file // os specific
}

Maybe we should adopt one of these

[makramkd]

Or maybe leverage sync.Mutex not being copyable:

type nonCopyable struct {
    _ sync.Mutex
}

type ccipChainReader struct {
  nonCopyable
// .. rest
}

[makramkd]

If you remove the pointer-to-interface this can be simplified to

Suggested change

	contracts, err := (*cdp.reader).DiscoverContracts(ctx, cdp.dest)
	contracts, err := cdp.reader.DiscoverContracts(ctx, cdp.dest)

[makramkd]

Suggestion for future PRs: maybe have a special constructor for the observation that is expected to be returned in each state, e.g NewGetCommitReportsObservation which would only take two args in this case groupedCommits and discoveryObs to avoid this nil, nil, nil dance which is not super readable. Alternatively we could instantiate the struct directly Observation{CommitReports: groupedCommits, DiscoveryObs: discoveryObs}

[asoliman92]

Isn't this what the initialized state do?

[winder]

The contracts are observed and updated every round, the initialized flag only prevents the other observation logic from running before the readers have been configured.

[asoliman92]

wrong comment?

[winder]

This is referring to the type. I'm not including the contracts in the final outcome.

[asoliman92]

Need to check that the Outcome actually did sync contracts before initializing here?

[winder]

The idea was that no error means that everything has initialized.

[github-actions]

Test Coverage

Branch	Coverage
will/onramp-observations-pt4	70.6%
main	70.4%