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.

Sign up for GitHub

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

Jump to bottom

Discover Router and FeeQuoters #171

Merged
merged 7 commits into from
Sep 30, 2024
Merged

Discover Router and FeeQuoters #171

merged 7 commits into from
Sep 30, 2024

Conversation

winder
Copy link
Contributor

@winder winder commented Sep 26, 2024 •
edited
Loading

Extend the DiscoverContract function:

  • Accepts a slice of chains. In the future this should be the total list of all supported chains.
  • Best-effort discovery of addresses known to the onramps. These will not be discovered until the second round.
  • Lookup additional offramp contracts: FeeQuoter, Router.

@winder winder force-pushed the will/discover-more-contracts branch 4 times, most recently from 23a6f5e to 790ba82 Compare September 27, 2024 16:16
makramkd
makramkd reviewed Sep 30, 2024
View reviewed changes
pkg/reader/ccip.go Outdated Show resolved Hide resolved
pkg/reader/ccip.go Outdated Show resolved Hide resolved
pkg/reader/ccip.go
) (ContractAddresses, error) {
if err := validateExtendedReaderExistence(r.contractReaders, destChain); err != nil {
return nil, err
// Exit without an error if we cannot read the destination.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We check for ErrContractReaderNotFound in the processor already - is that check still useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to DiscoverContracts along with a comment. The handling is a little different than elsewhere because it's important to continue after this error in order to handle cases where a node is configured such that they can read from a source chain but not the dest chain.

makramkd
makramkd reviewed Sep 30, 2024
View reviewed changes
internal/plugincommon/discovery/discoverytypes/types.go Show resolved Hide resolved
internal/plugincommon/discovery/discoverytypes/types.go
FChain map[cciptypes.ChainSelector]int
Addresses map[string]map[cciptypes.ChainSelector][]byte

// TODO: fix circular dependency /w token reader
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can move ContractAddresses to a new package reader/readertypes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think TokenReader should probably not be depending on something in the plugin packages

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh nvm I read token reader wrong. How does token reader get affected by all this? 🫨

Copy link
Contributor Author

@winder winder Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The token reader types are defined in the exectypes package and used by the pkg/reader/usdc_reader.go. They should probably be moved into the reader package, or the reader moved to exec.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cc @mateusz-sekara

internal/plugincommon/discovery/processor.go Show resolved Hide resolved
asoliman92
asoliman92 reviewed Sep 30, 2024
View reviewed changes
internal/plugincommon/discovery/processor_test.go Outdated
@@ -41,20 +43,31 @@ func TestContractDiscoveryProcessor_Observation_SupportsDest_HappyPath(t *testin
expectedOnRamp := map[cciptypes.ChainSelector][]byte{
source: []byte("onRamp"),
}
expectedFeeQuoter := map[cciptypes.ChainSelector][]byte{
source: []byte("onRamp"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding source as onRamp here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fee quoter from the onramp, the other entry here is a fee quoter from the destination. I updated the names for clarity.

asoliman92
asoliman92 reviewed Sep 30, 2024
View reviewed changes
pkg/reader/ccip.go
// OnRamps are in the offramp SourceChainConfig.
sourceConfigs, err := r.getSourceChainsConfig(ctx, chains)
// OnRamps are in the offRamp SourceChainConfig.
sourceConfigs, err := r.getOffRampSourceChainsConfig(ctx, allChains)
Copy link
Contributor

Choose a reason for hiding this comment

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

took me some time to get why we need the allChains here. Maybe add a doc to the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment on DiscoverContract. But also, this should hopefully go away soon.

@winder winder force-pushed the will/discover-more-contracts branch 2 times, most recently from 9a3b386 to 68f5a76 Compare September 30, 2024 15:37
winder
winder commented Sep 30, 2024
View reviewed changes
pkg/consts/consts.go
Comment on lines +61 to +68
// Deprecated: TODO: remove after chainlink is updated.
MethodNameOfframpGetDynamicConfig = "OfframpGetDynamicConfig"
// Deprecated: TODO: remove after chainlink is updated.
MethodNameOfframpGetStaticConfig = "OfframpGetStaticConfig"
// Deprecated: TODO: remove after chainlink is updated.
MethodNameOnrampGetDynamicConfig = "OnrampGetDynamicConfig"
// Deprecated: TODO: remove after chainlink is updated.
MethodNameOnrampGetStaticConfig = "OnrampGetStaticConfig"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing the casing on these, it will take a couple PRs.

@winder winder marked this pull request as ready for review September 30, 2024 18:04
@winder winder requested a review from a team as a code owner September 30, 2024 18:04
@winder
Discover Router and FeeQuoters on source chains.
6472a33 
Add a test for the ignored source fee quoter calls.

Make contract observation more generic.

Update another test.

fix cyclo lint warning.

Minor cleanup.

Add missing const, disable router discovery.

Disable router tests.

Add deprecated types back in.

lint
@winder
cleanup
0f0569a
@winder
Fix F.
c55e482
@winder
Lint
5395095
@winder
Add comment about allChains
fb1b081
@winder
Add more info.
2ef7d5a
@winder
Add OffRamp
f49d689
@github-actions GitHub Actions
Copy link

Metric will/discover-more-contracts main
Coverage 72.3% 71.9%

@winder winder mentioned this pull request Sep 30, 2024
Merged
@winder winder merged commit 4248922 into main Sep 30, 2024
4 checks passed
@winder winder deleted the will/discover-more-contracts branch September 30, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asoliman92 asoliman92 asoliman92 left review comments

@makramkd makramkd makramkd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@winder @asoliman92 @makramkd