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.

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

Ccip-3398 populate state initial PR #14391

Merged
merged 31 commits into from
Sep 20, 2024
Merged

Ccip-3398 populate state initial PR #14391

merged 31 commits into from
Sep 20, 2024

Conversation

AnieeG
Copy link
Contributor

@AnieeG AnieeG commented Sep 10, 2024

Adds snapshot creation for a set of contracts. The state snapshot can be updated later based on what more fields are required for each one. This is just to set the standard approach to be followed for state extraction.

  • TokenAdminRegistry
  • FeeQuoter
  • NonceManager
  • Router
  • RMN
  • OnRamp

Rest of the contracts will be covered on subsequent PRs.

@AnieeG AnieeG requested a review from a team as a code owner September 10, 2024 19:04
@@ -219,7 +213,7 @@ func LoadChainState(chain deployment.Chain, addresses map[string]deployment.Type
if err != nil {
return state, err
}
state.RMNRemote = rmnRemote
state.RMNRemote = rmn1_6.New(rmnRemote)
Copy link
Collaborator

Choose a reason for hiding this comment

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

how come rmn1_6 if this case statement is pointing to Version1_0_0?

case deployment.NewTypeAndVersion(RMNRemote, deployment.Version1_0_0...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I will update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ogtownsend can you point me to the code ref ? I see that TypeAndVersion for RMN is RMNRemote 1.6.0-dev

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 see what you mean, @connorwstein I think the case statement should be changed to Version1_6_0_dev, am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah should be changed

)

type Chain struct {
DestinationChainSelectors []uint64 `json:"destinationChainSelectors,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry about caching state from specific contracts at this top level - hard to follow where it comes from. Can we keep the state from each contract in the contract specific view so that its explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed DestinationChainSelectors in OnRamp and feeQuoter as of now to populate corresponding config. Do you suggest we pass router struct in the corresponding snapshot function to retrieve the chain selectors by calling getOffRamps?

DestinationChainSelectors []uint64 `json:"destinationChainSelectors,omitempty"`
// TODO - populate supportedTokensByDestination
SupportedTokensByDestination map[uint64][]string `json:"supportedTokensByDestination,omitempty"`
TokenAdminRegistry map[string]v1_5.TokenAdminRegistry `json:"tokenAdminRegistry,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting - I wasn't imagining we'd have multiple deployments of the same version of a contract on the same chain at any given time, but I guess its still possible so good to keep that flexibility.

// TODO - populate supportedTokensByDestination
SupportedTokensByDestination map[uint64][]string `json:"supportedTokensByDestination,omitempty"`
TokenAdminRegistry map[string]v1_5.TokenAdminRegistry `json:"tokenAdminRegistry,omitempty"`
FeeQuoter map[string]v1_6.FeeQuoter `json:"feeQuoter,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the json marshalling would still work if we kept keys as common.Address right because they have a String() method? Stronger typing there would prevent unexpected keys like "blah"

@@ -0,0 +1,33 @@
package types
Copy link
Contributor

Choose a reason for hiding this comment

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

idk if the types directory is helping much - think we could just have

view/chain.go
view/v1_2/..
view/v1_5/...

Copy link
Contributor Author

@AnieeG AnieeG Sep 13, 2024

Choose a reason for hiding this comment

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

The type directory is to avoid import cycle, We have the Contract struct getting imported into v1_2 ,v1_5 etc from parent view and then Chain struct has elements from v1_2, v1_5 in the parent view package

Copy link
Contributor

Choose a reason for hiding this comment

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

I see - I think it could be on the same hierarchical level though just to avoid the nesting

Address string `json:"address,omitempty"`
}

type ContractState interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

unused interface?

types.Contract
WrappedNative string `json:"wrappedNative,omitempty"`
ARMProxy string `json:"armProxy,omitempty"`
OnRamps map[uint64]string `json:"onRamps,omitempty"` // Map of DestinationChainSelectors to OnRamp Addresses
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the views are supposed to be human readable, I think we want to resolve the chain selectors into chain names 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.

Chain name derived from chain-selectors repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

on second thought, let's think more about how to do this in a uniform way for both selectors and addresses. Can leave it like this for now

}

func TokenAdminRegistrySnapshot(taContract *token_admin_registry.TokenAdminRegistry) (TokenAdminRegistry, error) {
tokens, err := taContract.GetAllConfiguredTokens(nil, 0, 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

will be an interesting problem here, there could be an unbounded number of tokens since its permissionless. We can start by just listing them all but lets add a TODO here for smarter pruning/DoS protection later.

onRamps[selector] = onRamp.Hex()
}
return Router{
Contract: types.Contract{
Copy link
Contributor

Choose a reason for hiding this comment

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

Owner is a pretty widespread field we can include that in the Contract type too I think

if err != nil {
return TokenAdminRegistry{}, err
}
return TokenAdminRegistry{
Copy link
Contributor

Choose a reason for hiding this comment

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

we definitely want to display the registry modules too. Sadly we have to filter events for that

TokenDecimals uint8 `json:"tokenDecimals,omitempty"`
}

func FeeQuoterSnapshot(fqContract *fee_quoter.FeeQuoter, tokensByDestination map[uint64][]string) (FeeQuoter, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer just only gethwrapper arguments, makes it explicit which contracts the view has dependencies on

connorwstein
connorwstein previously approved these changes Sep 20, 2024
}

func NewContractMetaData(tv Meta, addr common.Address) (ContractMetaData, error) {
tvStr, err := tv.TypeAndVersion(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

these might not always exist (e.g. WETH) but we can come back to that later, can just be empty in that case

@@ -49,6 +54,63 @@ type CCIPChainState struct {
TestRouter *router.Router
}

func (c CCIPChainState) GenerateView() (view.ChainView, error) {
chainView := view.NewChain()
r := c.Router
Copy link
Contributor

Choose a reason for hiding this comment

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

ultranit - we can avoid these variables and just use c.Router then if c.Router != nil {c.Router.Address()}

connorwstein
connorwstein previously approved these changes Sep 20, 2024
@cl-sonarqube-production
Copy link

@AnieeG AnieeG added this pull request to the merge queue Sep 20, 2024
Merged via the queue into develop with commit 674605e Sep 20, 2024
159 checks passed
@AnieeG AnieeG deleted the ccip-3398-populate-state branch September 20, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants