-
Notifications
You must be signed in to change notification settings - Fork 315
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(py): configurable eth account wallet provider rpc_url #474
base: main
Are you sure you want to change the base?
feat(py): configurable eth account wallet provider rpc_url #474
Conversation
🟡 Heimdall Review Status
|
@@ -42,7 +43,7 @@ def __init__(self, config: EthAccountWalletProviderConfig): | |||
self.account = config.account | |||
|
|||
chain = NETWORK_ID_TO_CHAIN[CHAIN_ID_TO_NETWORK_ID[config.chain_id]] |
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.
will this line fail with a non-explicitly supported chain ID?
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.
yes, absolutely. think we should also support a user defined chain within the config too?
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.
enabled custom chain definitions as well!
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.
Per John's comment, chain =
assignment will fail for a non-supported chainId.
However, chain
is only used to get the rpc_url
, which you are now passing in. Meaning, that line is now optional.
I think there's a cleaner way to write this that avoids the failure John is concerned about.
Something like:
rpc_url = config.rpc_url
if rpc_url is None:
chain = NETWORK_ID_TO_CHAIN[CHAIN_ID_TO_NETWORK_ID[config.chain_id]]
rpc_url = chain.rpc_urls["default"].http[0]
What changed?
enabling users to specify the rpc_url for the eth account wallet provider
Why was this change implemented?
Network support
Wallet support
Checklist
How has it been tested?
with invalid user defined rpc_url:
with valid user define rpc_url:
with user defined chain:
Notes to reviewers