Error handling: Frontend and Extension

[bonomat]

There is a need for improving the error handling in waves. Something like this is not really helpful :D

Incomplete list of error sources we need to deal with:

• interacting with bobtimus: when sending/receiving something to/from bobtimus things can go wrong. We should act on those errors in a useful way
• interacting with the wallet: when interacting with the wallet via the in-page script quite a few things can go wrong.
• UI errors: the user should get some feedback if he does something wrong on the UI (e.g. invalid inputs)

Note: if something goes wrong in the extension/wallet-lib the extension is basically dead.

[thomaseizinger]

Progress report 1:

Bobtimus API is mostly fine. Errors are handled cleanly at the moment. To get more fine-grained error reporting of individual things that fail to the client, we need to create more named errors (i.e. structs that implement std::error::Error) and use those in .context. Then we can react to those errors in problem::from_anyhow and return a clear problem to the client.

[thomaseizinger]

Progress report 2:

I had a look at the communication between in-page script, content-script, background-script and wasm-wallet. I think this can be made more resilient (which comes with better error reporting) if we employ different designs.

Here are my findings (in order of which I would tackle them):

1. Use randomly generated events to mimic request-response between in-page script and content-script

At the moment, in-page script and content-script use a generic "message" event to pass things back and forth. This doesn't really scale very well because it means that all message handlers need to be updated as soon as new event type is added, plus the implementation of each function within WavesProvider contains a lot of duplicated code.

We can solve this by using the JavaScript Proxy object to provide a generic implementation of any function call on an object. This generic implementation has access to the function name and parameters. Based on these, we can dispatch a message that looks like this:

window.postMessage({
	kind: '<function name>', // f.e. get-wallet-status
	args: {}
	responseEvent: '<random UUID>'
})

responseEvent is a randomly generated UUID. This allows us to register a handler using:

window.addEventListener("<random UUID>", listener);

Given this unique mapping, we can avoid carrying along a MessageKind in the response events because there is now a unique 1-to-1 mapping between posted messages and received ones.

Not only does this allow for a generic implementation and saving code, it is also more robust. The current implementation is susceptible to race-conditions if the same event is dispatched within a very short amount of time.

2. Make proper use of Promise-return types in background script

At the moment, the background script defines an async function handler, even though this is discouraged. Additionally, we use await on all the promises which triggers an exception if the promise rejects. Listeners of the onMessage property can deal with Promise return types (it is in fact encouraged to do that). As such, by removing all await key-words and directly returning the Promise, we will be able to forward any error that happens directly to the content script.

Together with (1), we no longer need to set the message response kind which should greatly simplify this function and reduce the risk of errors being dropped or not reported.

3. Directly listen for messages in Rust

At the moment, we use wasm_bindgen to expose functions through wasm to JavaScript. These functions are then used within the background script to react to messages via runtime.onMessage. In other words, the flow is:

We could remove a lot of boilerplate (wasm_proxy and the entire background script) if we would register the message listener directly inside Rust instead of exposing functions, i.e. do something like:

#[wasm_bindgen(start)]
pub fn setup() {
    /// ... other setup code

	/// This doesn't work exactly like this but that is not that important
    browser.runtime().on_message().add_listener(|message, source| {
		if message.type == 'get-wallet-status' {
			// return wallet.get_status()
		}
	});
}

It would be fairly trivial to write a proc-macro so the entire things looks like this:

#[message_handler]
pub async fn get_wallet_status() -> WalletStatus {
	// wallet.lock().await.get_status()
}

The new flow would be:

(3) Would also affect the background proxy but I don't think that is much of a big deal. The runtime.sendMessage API supports async/await hence there isn't much difference between calling the background script API directly or via runtime.sendMessage if they have to be define only once.

[bonomat]

Thanks @thomaseizinger :)

some thoughts/questions:

1. Awesome, I'm curious how 1 and 2 turn out. Using UUIDs for identifying events sounds like a great choice.

2. Regarding returning a promise in the eventlistener in the background script:
   Shouldn't there be an additional layer in between or do you plan to return the promise you get back from the actual function (e.g. walletStatus)?
   Because if an error gets thrown by walletStatus (which is in the wasm lib) it would get propagated all the way to the webpage/in-page script.

3. Regarding using listeners in Rust:
   This brings up some bad memories and I'm a kind of afraid of this approach. @luckysori and I were so happy when we moved away from having to write this logic in Rust ... like Christmas and Easter in one day :D
   I randomly picked a version from the past here so that you can have a look how it used to look like: https://github.com/comit-network/waves/blob/2ace9a9a4ab6676482845c9da16987216b008714/extension/background/src/lib.rs
   We had our own wasm_bindgen_extension library which provided additional wasm_bingens (here).
   It was super cumbersome to write and maintain in general and we didn't see much benefits of using Rust here because we immediately wrapped it into generic JsValues which removed any type-safetyness beyond the actual library call.
   Maybe it gets better if we can return the promise directly and don't need to do any conversions but I remember that this part was very painful.

[thomaseizinger]

3. Regarding using listeners in Rust:
   This brings up some bad memories and I'm a kind of afraid of this approach. @luckysori and I were so happy when we moved away from having to write this logic in Rust ... like Christmas and Easter in one day :D
   I randomly picked a version from the past here so that you can have a look how it used to look like: 2ace9a9/extension/background/src/lib.rs
   We had our own wasm_bindgen_extension library which provided additional wasm_bingens (here).
   It was super cumbersome to write and maintain in general and we didn't see much benefits of using Rust here because we immediately wrapped it into generic JsValues which removed any type-safetyness beyond the actual library call.
   Maybe it gets better if we can return the promise directly and don't need to do any conversions but I remember that this part was very painful.

I am well aware of that which is why it is the last item in the last of what I would tackle.

The benefits of doing this are not so much to make better use of Rust's type-system. The benefits would be cutting out the dumb pass-through layers of "wasm-proxy" and "background script" by directly registering the onMessage listener that is fired by the content-script in Rust.

I would write a proc-macro that generates all this ugly boilerplate for you at compile-time. If this pans out as intended, all you need to write is:

#[message_handler]
pub async fn get_wallet_status() -> WalletStatus {
	// wallet.lock().await.get_status()
}

To get the equivalent of:

browser.runtime.onMessage.addListener(message => {
	if message.type == 'get_wallet_status' {
		// wallet.lock().await.get_status()
	}
});

2. Regarding returning a promise in the eventlistener in the background script:
   Shouldn't there be an additional layer in between or do you plan to return the promise you get back from the actual function (e.g. walletStatus)?
   Because if an error gets thrown by walletStatus (which is in the wasm lib) it would get propagated all the way to the webpage/in-page script.

Isn't that what we want?

If we also achieve (3), then all we need to do is:

• define a schema for what errors look like in these onMessage channels
• define some infrastructure in Rust that converts errors to these schema

That would give you good control over which exact errors you want to return from the wasm-wallet and they would pop out in a nice format in the in-page script without any further doing.

[thomaseizinger]

I wrote up a summary of the current situation and problems I see with our approach of importing the wasm code: #228

I think a lot of our error handling problems stem from having all these pass-through layers. A simpler solution would very likely drastically improve this situation.

I've also created a follow-up issue for how to proceed with bobtimus' API: #229

As for:

UI errors: the user should get some feedback if he does something wrong on the UI (e.g. invalid inputs)

We can open up a separate issue for that as well, although I am not sure how important it is to separately track? It sounds very specific to the design of particular pages and forms, hence I am not sure if there is anything general to fix here. I think it would be best to record and fix those as independent issues.

Closing this as a result.