Hi,
I missed the last few messages, so sorry for this long reply.
@davidmurdoch, you made some really good points in your last few messages! I agree with you that there are some things that should be out of scope of this proposal.
Modifying the DOM being part of an RFC seems right. Also, I think that how to feature detect if a provider implements this proposal should probably be discussed there too.
Keeping this proposal focused just on the API of the Ethereum Provider would make it move forward much faster.
I proposed the isEIP1193
field because it would be useful for dapp and libraries authors, but I agree with removing it.
I’m not sure how detection would work without it. Checking for send
and on
function would incorrectly tell you that a MetaMask provider as injected today implements this EIP.
Anyway, I don’t think this should be discussed here nor mentioned in the EIP, but in another document.
I’m interested in this EIP because Buidler exposes a provider to their users. Unlocking accounts doesn’t make sense there either.
I think we could make the initial locked state of the accounts optional. With some explanation that it is a security sensitive decision.
Probably some clarification about accountsChanged
should be added in case of the provider starting with its accounts unlocked. Should it be emitted anyway? When? I think emitting events at arbitrary times (e.g. when initializing it) will lead to race conditions. When should dapp developers subscribe to it?
If we agree that exposing the provider to dapps should be treated in another document, I think the initial state of accounts should also be there. It seems to be a dapp browsers and wallet extensions specific problem.
I agree. Having that method should be up to the implementation. Some implementations would end up extending from EventEmitter
and exposing it, and that’s ok. But there’s no need to forcing all of them to expose it.
I think having an off
method would be nice to have a more consistent API. My concern is that many implementations would extend node’s EventEmitter
, and it only has off
in node 10+.
Maybe we should go with addListener
& removeListener
which have been available since EventEmitter
was added. The thing is that on
is far more used than addListener
, so on
& removeListener
doesn’t seem like a bad option to me.
The last thing I’d like to comment is that the proposal assumes that the provider has a sateful connection, and requires from it to emit connect
and close
events. There’s no such a thing as “connection” in Buidler, and AFAIK there isn’t in ganache-core
.
Also, as I mention before, I think firing events in arbitrary times (e.g. when the internal provider connection is open) will lead to race conditions.
I think an optional section for stateful providers can be added, or another document can extend this one with that.