near / mpc

37 stars 11 forks source link

Consider using `Base64VecU8` for `payload` arg when calling `sign` #578

Closed mikedotexe closed 2 months ago

mikedotexe commented 5 months ago

Description

It may be advantageous for the end user to have a more human-readable payload as they're confirming their transaction to the sign method, which is currently taking bytes:

https://github.com/near/mpc-recovery/blob/e89ff439ead0c2fc8e53043625241ece38dad2b8/contract/src/lib.rs#L337

An interesting aspect of this project is the current interaction between the end user and the MPC service, where the end user proffers a sighash payload to sign. From a UX perspective, this is a classic "damned if you do, damned if you don't" situation. This is particularly useful for Alice, who uses the MPC service for time-locked transactions, her preferred use case. The step right before calling sign involves the end user confirming with their wallet a seemingly random payload.

As we progress in this MPC effort, we'll likely be improving UX such that the end user can, themselves, verify the payload themselves. (This is not a ticket to create that part yet, it's setting it up for that future.)

If we have the payload argument be Base64VecU8 then the payload becomes more readable/recognizable than an array of bytes.

https://docs.rs/near-sdk/latest/near_sdk/json_types/struct.Base64VecU8.html

We can swiftly turn this into the byte array. Could even be a new function, sign_b64 that immediately calls sign if we want to avoid breaking changes.

mikedotexe commented 4 months ago

Ran into a weird thing that I wanted to share. I thought I kept doing something dumb, but turns out I didn't realize JavaScript can act different when using Array functions like include.

Screenshot 2024-05-21 at 4 26 56 PM

To test this, I quickly stored these values as hex, and the strangeness was indeed fixed. So it seems like there's a programmatic advantage to using a base64 string.

Reference: https://chatgpt.com/share/1d56c379-00cd-4d32-b5a2-83443c91a2de

ewiner commented 4 months ago

Reference equality for arrays is a pretty well-known behavior of Javascript. That is,

> [1, 2] == [1, 2];
false

> const foo = [1, 2];
> foo == foo;
true

I don't think we should modify the Rust API just because of strangeness in the JS language spec.

A benefit of the [u8; 32] type is that it's exactly 32 bytes, which serves as both a compile-time check and implicit documentation for the function. I wouldn't mind a new helper function that takes an argument that's more like a hex string, validates its length, then calls sign(), especially if it's helpful for JS interop. But that could also be accomplished outside the contract in library code.

mikedotexe commented 4 months ago

I wouldn't mind a new helper function that takes an argument that's more like a hex string, validates its length, then calls sign(), especially if it's helpful for JS interop. But that could also be accomplished outside the contract in library code.

Yes, this was the exact suggestion in this ticket from a month ago

Screenshot 2024-05-22 at 8 44 50 AM

This ticket takes less than 5 minutes given all the details here, shall I do it?

ewiner commented 4 months ago

Sure!