Updated Keyring Controller #85
Conversation
added methods create and import BLS accounts updated KeyringController interface added method to sign transactions
| } | ||
|
|
||
| private _getContractWalletAddress(privateKey: string) { | ||
| const provider = new ethers.providers.JsonRpcProvider(PROVIDER_URL); |
There was a problem hiding this comment.
Should this provider come from other controllers?
the SafeEventEmitterProvider used by other controllers isn't compatible with this
There was a problem hiding this comment.
It would be better to use one ethers.js provider and pass that down, as this may create additional socket connections/network request overhead. For now though to get it working should be okay.
Also, you use CHAIN_RPC_URL instead of adding a new env var. PROVIDER_URL === CHAIN_RPC_URL.
| const provider = new ethers.providers.JsonRpcProvider(PROVIDER_URL); | |
| const provider = new ethers.providers.JsonRpcProvider(CHAIN_RPC_URL); |
There was a problem hiding this comment.
it should be the provider coming from other controllers.
it is an eip-1193 compatible provider.
| } | ||
|
|
||
| private _getContractWalletAddress(privateKey: string) { | ||
| const provider = new ethers.providers.JsonRpcProvider(PROVIDER_URL); |
There was a problem hiding this comment.
It would be better to use one ethers.js provider and pass that down, as this may create additional socket connections/network request overhead. For now though to get it working should be okay.
Also, you use CHAIN_RPC_URL instead of adding a new env var. PROVIDER_URL === CHAIN_RPC_URL.
| const provider = new ethers.providers.JsonRpcProvider(PROVIDER_URL); | |
| const provider = new ethers.providers.JsonRpcProvider(CHAIN_RPC_URL); |
| private async _getBLSSinger() { | ||
| return initBlsWalletSigner({ | ||
| chainId: Number(this.state.chainId), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Instead of initing and handling signing via the BLS signer directly, you may just want to use the BlsWalletWrapper abstraction instead and add it into the state. I am unsure if the state needs to have only primitively typed data in it, if so you may be able to use another controller/construct to manage BlsWalletWrapper wallet objects.
You then would only need to call BlsWalletWrapper.connect once and then store the wallet object it returns and use that for signing/address operations. See https://github.com/jzaki/bls-wallet/tree/main/contracts/clients#blswalletwrapper for details.
There was a problem hiding this comment.
semi-fixed in f30cd85
Yeah I agree. As the prev comment mentions, should use the provider coming from the state. Couldn't because of incompatibility. Would be nice to get @chaitanyapotti's opinion on how to pass the provider to each controller.
Also @jacque006, the wrapper currently has a private constructor and static connect and address methods. It wont be possible to create an object (with only provider and verification address), save it in state and just use different privKeys to do different ops. WDYT about adding these to the client library itself?
There was a problem hiding this comment.
connect is essentially the async constructor for BLSWalletWrapper as it returns an initialized BLSWalletWrapper object. You would need to connect for each private key and store the wrapper object for each wallet.
I think rolling with the static methods for now is fine per our offline conversation. But it may be easier in the future to have a common place the BLSWalletWrapper object(s) can be accessed from.
|
@kautukkundan I see you checked "I have manually tested these changes", but I don't understand what manual testing you could have done and you haven't written anything for that section. Could you clarify? |
There was a problem hiding this comment.
Looks sensible to me. I think you need to implement @jacque006's point about a shared ethers provider though and use CHAIN_RPC_URL instead of creating a new environment variable.
I'd also like @chaitanyapotti to give this a once-over since the target is his wip branch. I also imagine he'd be your best bet for doing the shared provider if there's any complications with that.
Once those things are taken care of let's get this merged.
updated wallet-client version removed extra env var PROVIDER_URL
|
ah @voltrevo manually tested on local terminal by importing modules and checking the returned values. |
Ohh nice 👍 |
|
ethers is a very heavy library. |
What is this PR doing?
updated bls-wallet-clients version
added methods create and import BLS accounts
updated KeyringController interface
added method to sign transactions
How can these changes be manually tested?
Does this PR resolve or contribute to any issues?
Checklist
Guidelines
resolve conversationbutton is for reviewers, not authors