-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Allow to hydrate contract from the capability registry view #15309
Conversation
ChainID uint64 | ||
} | ||
|
||
func (v CapabilityRegistryView) Hydrate(env deployment.Environment, cfg HydrateConfig) (*deployment.ContractDeploy[*capabilities_registry.CapabilitiesRegistry], error) { |
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.
the views packages should all be readonly. Anything that writes onchain should be in a changeset. You can add a common/changeset if you want though, after #15288 there will already be a directory for that you can use
) | ||
return deployment.ContractDeploy[*capabilities_registry.CapabilitiesRegistry]{ | ||
Address: crAddr, Contract: cr, Tx: tx, Err: err2, | ||
Tv: deployment.NewTypeAndVersion("CapabilitiesRegistry", deployment.Version1_0_0), |
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.
these values come from the contract itself
return nil, fmt.Errorf("failed to convert nodes to nodes params: %w", err) | ||
} | ||
tx, err := deployedContract.Contract.AddNodes(chain.DeployerKey, nodesParams) | ||
if _, err = deployment.ConfirmIfNoError(chain, tx, err); err != nil { |
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.
use this helper to decode the err parse err func
} | ||
|
||
capabilitiesParams := v.CapabilitiesToCapabilitiesParams() | ||
tx, err = deployedContract.Contract.AddCapabilities(chain.DeployerKey, capabilitiesParams) |
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.
need to add both nops & capabilities before adding nodes b/c FK constraints in the contract enforce the nop id and hash capabilities values already exist
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
var capabilityRegistryView v1_0.CapabilityRegistryView | ||
require.NoError(t, json.Unmarshal(b, &capabilityRegistryView)) | ||
|
||
chainID := chainsel.TEST_90000001.EvmChainID + 1 |
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.
why + 1
?
i think there are multiple TEST selectors if you simply want one that is not 90000001
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.
Used +1 based on how chains are generated here.
) | ||
|
||
func DeployCapabilityRegistry(env deployment.Environment, config interface{}) (deployment.ChangesetOutput, error) { | ||
func DeployCapabilityRegistry(env deployment.Environment, config interface{}) (deployment.ChangesetOutput, *capabilities_registry.CapabilitiesRegistry, error) { |
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 will break downstream users.
there ought to be a type assert here, but it's missing. please add it
type assertion
instead of returning the contract, you can load the contract from the returned addressbook
contract set
return nil, fmt.Errorf("failed to deploy contract: %w", err) | ||
} | ||
|
||
nopsParams := v.NopsToNopsParams() |
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.
@connorwstein it's not straightforward to reuse the existing changeset that deploys/configures our capability registry because it does too much stuff (and need to be refactored to support mcms as discussed in slack)
i'd like to move forward with this approach here so we can use the tool downstream
@vyzaldysanchez please file a ticket to refactor this logic into calls to bite-sizes changesets. and write in that ticket that we need to implement the changesets first.
Requires
Supports