-
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
[NONEVM-904] Update address book to support nonevm #15269
Changes from 4 commits
58c3eb6
c10af69
4132479
1ad01a2
9c2398d
b5b95e1
5fc8f07
a77e91e
ad98911
051be36
374ee75
409e9ba
122241f
f013d31
0a2ef89
fb24bb7
986a6a4
3ba1ff2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"chainlink": minor | ||
--- | ||
|
||
Update deployment address book to support non-evm chains |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import ( | |
func NewWriteTarget(ctx context.Context, relayer *Relayer, chain legacyevm.Chain, gasLimitDefault uint64, lggr logger.Logger) (*targets.WriteTarget, error) { | ||
// generate ID based on chain selector | ||
id := fmt.Sprintf("write_%[email protected]", chain.ID()) | ||
// NOTE: This does not support non-evm chains | ||
chainName, err := chainselectors.NameFromChainId(chain.ID().Uint64()) | ||
if err == nil { | ||
id = fmt.Sprintf("write_%[email protected]", chainName) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,20 +93,22 @@ type AddressBookMap struct { | |
mtx sync.RWMutex | ||
} | ||
|
||
// save will save an address for a given chain selector. It will error if there is a conflicting existing address. | ||
func (m *AddressBookMap) save(chainSelector uint64, address string, typeAndVersion TypeAndVersion) error { | ||
_, exists := chainsel.ChainBySelector(chainSelector) | ||
if !exists { | ||
// Save will save an address for a given chain selector. It will error if there is a conflicting existing address. | ||
func (m *AddressBookMap) Save(chainSelector uint64, address string, typeAndVersion TypeAndVersion) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's update the Save test with some other family addresses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did this become public? i don't see anything that using it in other packages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. case added! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah +1 @krehermann this should remain a private helper, there's already a public Save. Not sure how this actually compiles it looks like it has the same signature as the public Save? |
||
family, err := chainsel.GetSelectorFamily(chainSelector) | ||
if err != nil { | ||
return errors.Wrapf(ErrInvalidChainSelector, "chain selector %d", chainSelector) | ||
} | ||
if address == "" || address == common.HexToAddress("0x0").Hex() { | ||
return errors.Wrap(ErrInvalidAddress, "address cannot be empty") | ||
} | ||
if common.IsHexAddress(address) { | ||
// IMPORTANT: WE ALWAYS STANDARDIZE ETHEREUM ADDRESS STRINGS TO EIP55 | ||
address = common.HexToAddress(address).Hex() | ||
} else { | ||
return errors.Wrapf(ErrInvalidAddress, "address %s is not a valid Ethereum address, only Ethereum addresses supported", address) | ||
if family == chainsel.FamilyEVM { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could add your aptos specific validation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't have any yet, but we'll create a ticket to add it later. We are already tackling hardening tasks like this one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was toying with an idea of building in aptos and other chain or family validations into the chainlink-selectors repo, so we'd have a set of reusable validations keyed off of chain ID in a way that hides all the conditionality from calling code. Valid addresses was definitely in the right zone. Doing it here is fine, but we should think about making this more generally usable. |
||
if address == "" || address == common.HexToAddress("0x0").Hex() { | ||
return errors.Wrap(ErrInvalidAddress, "address cannot be empty") | ||
} | ||
if common.IsHexAddress(address) { | ||
// IMPORTANT: WE ALWAYS STANDARDIZE ETHEREUM ADDRESS STRINGS TO EIP55 | ||
address = common.HexToAddress(address).Hex() | ||
} else { | ||
return errors.Wrapf(ErrInvalidAddress, "address %s is not a valid Ethereum address, only Ethereum addresses supported for EVM chains", address) | ||
} | ||
} | ||
if typeAndVersion.Type == "" { | ||
return fmt.Errorf("type cannot be empty") | ||
|
@@ -142,8 +144,8 @@ func (m *AddressBookMap) Addresses() (map[uint64]map[string]TypeAndVersion, erro | |
} | ||
|
||
func (m *AddressBookMap) AddressesForChain(chainSelector uint64) (map[string]TypeAndVersion, error) { | ||
_, exists := chainsel.ChainBySelector(chainSelector) | ||
if !exists { | ||
_, err := chainsel.GetChainIDFromSelector(chainSelector) | ||
if err != nil { | ||
return nil, errors.Wrapf(ErrInvalidChainSelector, "chain selector %d", chainSelector) | ||
} | ||
|
||
|
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.
Are these notes intended to be TODOs, or is this purely documentary?
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.
documentation