Adds event emitter to window.ethereum object #8718
Conversation
| resource_name = "brave_wallet_script" | ||
| } | ||
|
|
||
| pack_web_resources("generated_resources") { |
There was a problem hiding this comment.
This is a bit confusing with generated_resources for brave_wallet for the panel and the page.
Maybe call it ethereum_provider_generated_resources?
See
https://github.com/brave/brave-core/blob/master/components/brave_wallet_ui/BUILD.gn#L29
Mainly brave_wallet is not specific enough because it is used for a lot of things. We should say what it is specifically. Which is an ethereum provider.
components/resources/BUILD.gn
Outdated
| if (!is_android) { | ||
| deps += [ "//brave/components/brave_wallet_ui:resources" ] | ||
| sources += [ | ||
| "$root_gen_dir/brave/components/brave_wallet/resources/brave_wallet.pak", |
There was a problem hiding this comment.
now that we'll have 2 different pak files though I should come back and rename this to brave_wallet_ui.pak
|
linux CI randomly fails on diff rewards tests which are not related to the PR, last time it is |
|
ios and macos CI keep aborting |
| FormProviderErrorResponse(message).c_str())); | ||
| } | ||
|
|
||
| void BraveWalletJSHandler::ChainChangedEvent(const std::string& chain_id) { |
There was a problem hiding this comment.
I don't think we want methods for each specific event, we should just have a list of constants for them
There was a problem hiding this comment.
I will do that in upcoming PRs. As some events will come from browser process via mojo, some will not like connect, on connect we will ask a browser process what chain id is. That place is going to be changed in some kind in the upcoming PRs. Just some things are unknown yet till we investigate how and when exactly we should fire events.
There was a problem hiding this comment.
can we also add better error handling in a follow-up PR so the called gets notified if there are any JS errors?
| @@ -107,6 +150,9 @@ v8::Local<v8::Promise> BraveWalletJSHandler::Request( | |||
| if (!out || !out->is_dict() || !out->GetAsDictionary(&out_dict)) | |||
| return v8::Local<v8::Promise>(); | |||
|
|
|||
| // Hardcode id to 1 as it is unused | |||
| ALLOW_UNUSED_LOCAL(out_dict->SetIntPath("id", 1)); | |||
There was a problem hiding this comment.
these should probably be consts defined in brave_wallet/common or something
There was a problem hiding this comment.
I'm going to do it as consts in that cc as they are not supposed to be used anywhere else
SergeyZhukovsky
force-pushed
the
web3_event_emitter
branch
from
880c906 to
af6e4d0
Compare
| @@ -107,6 +175,9 @@ v8::Local<v8::Promise> BraveWalletJSHandler::Request( | |||
| if (!out || !out->is_dict() || !out->GetAsDictionary(&out_dict)) | |||
| return v8::Local<v8::Promise>(); | |||
|
|
|||
| // Hardcode id to 1 as it is unused | |||
| ALLOW_UNUSED_LOCAL(out_dict->SetIntPath("id", kRequestId)); | |||
There was a problem hiding this comment.
weird that these were needed, I don't see them used in other places, but I guess it's ok because other places also ignore the result
|
|
||
| namespace brave_wallet { | ||
|
|
||
| BraveWalletJSHandler::BraveWalletJSHandler(content::RenderFrame* render_frame) | ||
| : render_frame_(render_frame) {} | ||
| : render_frame_(render_frame) { | ||
| if (g_provider_script->empty()) { |
There was a problem hiding this comment.
this is fine for now, but I don't think we need to store this in a static var. LoadDataResource loads from memory so there's really no benefit to doing this
| } | ||
|
|
||
| deps += [ "//brave/components/brave_wallet/resources:ethereum_provider_generated_resources" ] | ||
| sources += [ "$root_gen_dir/brave/components/brave_wallet/resources/brave_wallet_script_generated.pak" ] |
There was a problem hiding this comment.
there's no reason to expose additional pak files here, we should repack everything inside brave_wallet into a single output. See https://github.com/brave/brave-core/pull/8703/files#diff-5303bb2b8878c4fce006fc3b8bb18f3d4e0f386dbea78d4c8c37a23323762389R55
|
post-init CI failed on not related to the PR audits checks |
Resolves brave/brave-browser#15636
Note: actual emit of events, tests and hooking up them to UI will be done in separate issues.
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: