ccip loopp - migrate to relayer types#489
Conversation
… into dk/ccip-loop-types
roman-kashitsyn
left a comment
roman-kashitsyn
left a comment
There was a problem hiding this comment.
Generally, LGTM.
It would be very helpful to include more background in the PR description. For example, the following points aren’t obvious:
- Why do we replace structured addresses with raw strings.
- Why do we move some (but not all?) internal CCIP interfaces to the
cciptypespackage. - Why we don’t use the
GasPricealias anymore.
core/services/ocr2/plugins/ccip/internal/ccipdata/factory/onramp.go
Outdated
Show resolved
Hide resolved
| // the fields below are initially empty and populated on demand | ||
| destPriceRegReader: nil, | ||
| destPriceRegAddr: common.Address{}, | ||
| destPriceRegAddr: "", |
There was a problem hiding this comment.
Shouldn't we have an empty cciptypes.Address instead for consistency (even if the result is the same)
There was a problem hiding this comment.
it's a type alias so it's either empty string or cciptypes.Address("") which is the more verbose version.
I believe it's ok with an empty string
| } | ||
| return common.HexToAddress(string(a)), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
We could have a fromEVM(..) method then, allowing to build an Address from an EVMAddress instead of having string conversions everytime.
|
Added some comments on the chainlink-common PR, I think changes there will affect this PR right? |
… into dk/ccip-loop-types
|
@dimkouv would it be simpler to point to the chainlink-common branch that you have your changes on instead of duplicating the types here? Once that PR gets merged you can update here w/ the new main branch of chainlink-common. |
Since we are migrating to the LOOPP architecture we want to start using primitive types in the core ccip logic. 1. Uses the generic types that are defined under chainlink-common 2. Misc refactoring (evm addresses to strings, etc..) 3. Move the interfaces that are exposed to the plugin under chainlink-common. For example the `Router interface` is not moved since the plugin does not use it directly. NOTE: Duplicated the types at `"github.com/smartcontractkit/chainlink/v2/core/services/ocr2/plugins/ccip/cciptypes"` which should be replaced by `"github.com/smartcontractkit/chainlink-common/pkg/types/cciptypes"` after smartcontractkit/chainlink-common#330 is merged.
4 participants
Since we are migrating to the LOOPP architecture we want to start using primitive types in the core ccip logic.
Router interfaceis not moved since the plugin does not use it directly.NOTE: Duplicated the types at
"github.com/smartcontractkit/chainlink/v2/core/services/ocr2/plugins/ccip/cciptypes"which should be replaced by"github.com/smartcontractkit/chainlink-common/pkg/types/cciptypes"after smartcontractkit/chainlink-common#330 is merged.