send dialog and plumbing does not handle over-long memos

[hdevalence]

1. generate a 512-byte memo plaintext, this is longer than the limit (the address and the text together are 512 bytes)

   Python 3.9.6 (default, Nov 10 2023, 13:38:27)
   [Clang 15.0.0 (clang-1500.1.0.2.5)] on darwin
   Type "help", "copyright", "credits" or "license" for more information.
   >>> 'aaaaaaa ' * (512//8)

2. paste into memo field in send dialog
3. tx approval popup appears, click approve
4. tx build fails with inscrutable error message

two problems here:

1. form should be doing input validation on memo length
2. errors should be propagated back to user

[turbocrime]

after some investigation, 'unreachable' is fully accurate: the wasm call throws a RuntimeError.unreachable. so as far as the extension and webapp can handle, propagation is correct.

we can wrap every impl's wasm calls in try/catch to add more detail, but it might be more correct to throw meaningful errors from wasm, either in wasm-ts, wasm-bundler, or from wasm itself

[turbocrime]

it's worth considering that errors may contain potentially sensitive information

connectrpc has chosen deliberately to obfuscate errors in implementations as generic "internal" errors, unless they are thrown specifically as a ConnectError: https://github.com/connectrpc/connect-es/discussions/341

in the course of #295 we took the opposite attitude, but in routing-refactor #282 this behavior of obfuscating internal errors resumes, as we're now 'properly' using connectrpc server tooling

i'm going to add catch/rethrows around every wasm call in routing-refactor, and convert existing throws to ConnectError throws, as those are clearly intended to be explicitly returned

we should identify errors that might leak information

[hdevalence]

Huh, that's very surprising. It seems like a very strange policy to me, since if a server is leaking sensitive info in its error messages that's a problem with the server, and censoring errors from one particular client doesn't really fix that.

[hdevalence]

i'm going to add catch/rethrows around every wasm call in routing-refactor, and convert existing throws to ConnectError throws, as those are clearly intended to be explicitly returned

Hmm, maybe we should wait to figure out if there's another solution before we go and add boilerplate code to all of our handlers. My instinct is that I don't think we should be doing any connect-specific error handling. We need to ensure that the extension's view server is just like any other, and that it would be possible (in principle) to swap it out for a network call to pclientd. The pclientd code will be using grpc-web, not connect, so it won't do any connect-specific error handling.

If this problem is caused by an upstream tooling choice, we should pause and wait to think through a solution before solving it, rather than adding wrappers around all of our code.

[turbocrime]

the choice is made to assist in correct implementation. you can still choose to throw from the service if you'd like, but the default is to obfuscate, and i think that's a good default.

upstream tooling is not causing a specific problem, the wasm implementation we currently have is failing to throw anything meaningful

my last comment was anticipating some issues in my refactor that i just noticed, and perhaps was better placed in the thread for that pr

[grod220]

the wasm call throws a RuntimeError.unreachable

Sadly, this likely means that the Rust code panicked. A part of addressing this task should mean finding the part of the Penumbra repo codebase that isn't using the Result/Err type (I suspect it's a .expect() somewhere) and fixing it.

so as far as the extension and webapp can handle

When I ran into this before, I had a hard time wrapping this in a try/catch on the javascript side. It's more like the whole process aborts.

but in routing-refactor https://github.com/penumbra-zone/web/pull/282 this behavior of obfuscating internal errors resumes, as we're now 'properly' using connectrpc server tooling

Obfuscating errors feels like a very painful road to continue on. This makes debugging user errors essentially impossible and turns troubleshooting into 🤷‍♂️ . I don't see why we wouldn't be more expressive in error messages and choose to intentionally throw ConnectError so that the web app can give meaningful feedback to users.

[turbocrime]

https://github.com/penumbra-zone/web/commit/ae2b7456aa6881046df924f43e78aa2120631471 in #282 catching all wasm calls in impls and logging all errors thrown out of impls

[turbocrime]

ae2b745 reverted. current state is:

• landed form validation
• not performing validation in js impl
• impl throws correctly propagate to client
• wasm error messages are not meaningful

[turbocrime]

length used in #329 was incorrect. wasm uses an 80-byte address in the memo, not a BECH32 address, so the boundary is 432/433

[turbocrime]

356 correct memo length in form validation

[turbocrime]

view service validation merged https://github.com/penumbra-zone/penumbra/pull/3585