-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: ensure the injectedRegsitry opt does deep comparisons #359
Conversation
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.
On the first review I have some nits, and questions. So far though, amazing job!
As mentioned in an offline discussion we are moving this PR from a override to injection only, and a less rigid set of rules, making it as much as a deep comparison as possible |
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.
lgtm!
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.
Just one little nit, then this is good to go! Really great job :)
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.
LGTM, really amazing job!
Description
It allows updating the entries of the existing registry with new info~, as well as overriding existing entries~. The injected registry no longer requires to have all the fields filled when updating an existing entry, but it errors out when adding a new entry and not specifying the chain's
specName
or.tokens
Added
updateRegistry
toparseRegistry
Added the function
updateRegistry
to handle checking whether the entry already exists or if it's new info, and if it's already present, it updates the corresponding fields.Added alternative interfaces with optional values
Added
InjectedChainInfoRegistry
,InjectedChainInfo
andInjectedChainInfoKeys
alternative interfacesto allow for injected registries with partial info, as well asthemadeAssetTransferApiInjectedOpts
type for comparisonAssetTransferApiOpts
,ChainInfo
andChainInfoRegistry
to take eitherInjectedChainInfoKeys
orChainInfoKeys
depending on what's needed.Updated tests and
adjustedMockSystemApi
Foreign assets' tests failed because
TNKR
is already in the registry, so I modified theadjustedMockSystemApi
'sforeignAssets
to another asset, updated the corresponding tests, and updated theparseRegistry
's tests to account for the new functionality.Rel: #357