-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
refactor and cleanup oracle creator to use plugin map #16363
base: develop
Are you sure you want to change the base?
Conversation
chainFamily string, | ||
offrampProgramAddress []byte, | ||
destChainSelector uint64, | ||
) (types.ContractWriter, error) | ||
} | ||
|
||
var plugins = map[string]plugin{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this is statically initializing the plugin struct, thus we are forced to have some types take in func which later on takes params like logger.
Does it make sense to NOT have this declared statically, and instead have the plugins as a member field in pluginOracleCreator, which is then initialized from within NewPluginOracleCreator(). Here we can create the plugin instances for all chain families.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And once we do that, then all the actual init for individual members inside plugin struct can just stay in their own chain package like ccipevm or ccipsolana.
This way, this package only imports ccipevm or ccipsolana packages, instantiates each component needed, like say NewCommitPluginCodec(), newExecPluginCodec(), NewCRConfig(), NewCWConfig() and so on.
Each of these above functions should live inside their own chain family package.
That would clean this package completely of chain families, except for just initially initializing the plugin struct for all chain families.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we can later clean this up further for sure, the current implementation was just a way to add non-EVM support in a small diff
TokenDataEncoder cciptypes.TokenDataEncoder | ||
GasEstimateProvider cciptypes.EstimateProvider | ||
RMNCrypto func(lggr logger.Logger) cciptypes.RMNCrypto | ||
EncodedOfframpAddr func([]byte, bool) string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is just serializing an address to string. So better to rename it AddressToString()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chainlink-ccip
also has these helpers I had to patch on my branch, they're currently marked as internal but maybe we could import them: https://github.com/smartcontractkit/chainlink-ccip/blob/main/internal/libs/typeconv/address.go
"chainlink": minor | ||
--- | ||
|
||
Cleanup and refactoring several functions to use the plugin map #added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the changeset check is technically not required. I usually skip changeset notes unless it's a larger user impacting change
AER Report: CI Coreaer_workflow , commit , Clean Go Tidy & Generate , Detect Changes , Scheduled Run Frequency , GolangCI Lint (.) , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Core Tests (go_core_fuzz) , test-scripts , Core Tests (go_core_race_tests) , Core Tests (go_core_ccip_deployment_tests) , lint , SonarQube Scan 1. GolangCI Lint job failed due to linting errors:
|
Let's delay merging this until some of the fixes from #15965 are merged or it'll cause conflicts on my branch |
|
Cleanup and refactoring several functions to follow the plugin convention introduced in PR.