Ccip-4506 log improvements in deployment code #15535
Conversation
Flakeguard Summary
Found Flaky Tests ❌
|
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
| @@ -286,16 +284,15 @@ func deployChainContracts( | |||
| } | |||
| }) | |||
| if err != nil { | |||
| e.Logger.Errorw("Failed to deploy RMNProxyNew", "err", err) | |||
| e.Logger.Errorw("Failed to deploy RMNProxyNew", "chain", chain.Selector, "err", err) | |||
| return err | ||
| } | ||
| e.Logger.Infow("Added nonce manager authorized callers", "chain", chain.Name, "callers", offRampContract.Address(), onRampContract.Address()) |
There was a problem hiding this comment.
doesn't the Infow one have to have the form "string", "key", value, "key", value...?
| return nil, err | ||
| } | ||
| lggr.Infow("deployed CCIPHome", "addr", ccipHome.Address) |
There was a problem hiding this comment.
I think we want all the positive logs too
There was a problem hiding this comment.
According to @AnieeG they were moved to DeployContract func
| @@ -267,7 +269,7 @@ func addChainConfig( | |||
| if _, err := deployment.ConfirmIfNoError(h, tx, err); err != nil { | |||
| return ccip_home.CCIPHomeChainConfigArgs{}, err | |||
| } | |||
| lggr.Infow("Applied chain config updates", "chainConfig", chainConfig) | |||
| lggr.Infow("Applied chain config updates", "homeChain", h.Name, "addedChain", chainSelector, "chainConfig", chainConfig) | |||
| return err | ||
| } | ||
| lggr.Infow("deployed weth9", "addr", weth.Address) |
There was a problem hiding this comment.
yeah I like the positive logs, let add em everywhere
There was a problem hiding this comment.
Added with deployContract - https://github.com/smartcontractkit/chainlink/blob/CCIP-4506-log-improvements/deployment/helpers.go#L149
deployment/environment.go
Outdated
| @@ -50,6 +50,7 @@ type OffchainClient interface { | |||
| type Chain struct { | |||
| // Selectors used as canonical chain identifier. | |||
| Selector uint64 | |||
| Name string | |||
There was a problem hiding this comment.
I worry this could be populated wrong, can we leave it as is and derive from selector?
There was a problem hiding this comment.
I can move the name population to deployment.environment ow for every function we will have to call the chaininfo function,
There was a problem hiding this comment.
could we just have a method on Chain called Name() which does the derivation?
deployment/environment.go
Outdated
| Client OnchainClient | ||
| // Note the Sign function can be abstract supporting a variety of key storage mechanisms (e.g. KMS etc). | ||
| DeployerKey *bind.TransactOpts | ||
| Confirm func(tx *types.Transaction) (uint64, error) | ||
| } | ||
|
|
||
| func (c Chain) Name() string { | ||
| return c.name |
There was a problem hiding this comment.
oh I was thinking of just inlining
func (c Chain) Name() string {
chainInfo, err := ChainInfo(c.Selector)
if err != nil {
// we should never get here, if the selector is invalid it should not be in the environment
panic(err)
}
return chainInfo.Name
}
its just a map lookup inside there anyways
|
I see you updated files related to
|
AnieeG
requested review from
winder,
rstout,
asoliman92,
makramkd,
dimkouv,
0xAustinWang and
b-gopalswami
…CCIP-4506-log-improvements
Requires
Supports