Crypto api, Rust-only

[maurolacy]

Closes first part of #751: secp256k1 signature verification for Cosmos signatures.

Alernative to #767, using the Rust-only crate k256.

Still some improvements to do, documentation to add, but ready for review.

[webmaster128]

Nice stuff

[webmaster128]

Some more comments. Will get back to the open question after lunch

[webmaster128]

Could we return bool instead of u32 as the return type of verify_secp256k1 over the FFI interface?

[maurolacy]

Could we return bool instead of u32 as the return type of verify_secp256k1 over the FFI interface?

Of course! Guess wasm32 memory model is messing with my brain's workings.

[maurolacy]

Could we return bool instead of u32 as the return type of verify_secp256k1 over the FFI interface?

It seems that's not possible without modifying / extending wasmer in some way:

the trait `wasmer::HostFunction<_, _, wasmer::internals::WithEnv, environment::Environment<A, _, _>>` is not implemented for `for<'r> fn(&'r environment::Environment<_, _, _>, u32, u32, u32) -> std::result::Result<bool, errors::vm_error::VmError>`

[webmaster128]

It seems that's not possible without modifying / extending wasmer in some way:

Okay, seems like there are only those value types in WebAssembly:

/// A list of all possible value types in WebAssembly.
#[derive(Copy, Debug, Clone, Eq, PartialEq, Hash)]
#[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]
pub enum Type {
    /// Signed 32 bit integer.
    I32,
    /// Signed 64 bit integer.
    I64,
    /// Floating point 32 bit integer.
    F32,
    /// Floating point 64 bit integer.
    F64,
    /// A 128 bit number.
    V128,
    /// A reference to opaque data in the Wasm instance.
    ExternRef, /* = 128 */
    /// A reference to a Wasm function.
    FuncRef,
}

See also https://github.com/WebAssembly/design/blob/master/Semantics.md#types.

Alright, let's keep the 0/1 solution then.

[maurolacy]

Yeah, I was just thinking that booleans are notoriously inconsistent, both in and across languajes / platforms.
Seems my brain's workings somehow "knew" / anticipated that.

[maurolacy]

StdResult wrapper is basically useless now. Do we go ahead and remove it? Or do we keep it, just for API consistency?

[webmaster128]

Let's remove it. The fewer errors the contract developer needs to handle, the better. We have many API calls that can only:

• Succeed
• Fail in VM and never return to contract
• Panic in contract

All the Storage API works like this.

[maurolacy]

Hmmm, it seems StdResult is a good idea here, for Api compatibility; i.e. that way the MockApi impl can return errors, in contract testing land.

Not sure if we want that, though. Though it will ease development / debugging, it points to functionality that will never trigger behaves differently in the "real" (vm) environment.

Update: I think it's nevertheless a good idea, for usability / transparency. Will add both versions, and we can decide.

What do you think?

[webmaster128]

Good point. I tend to favour panics in the MockApi. Those can easily debug and we optimize for the production use case where there is nothing the contract developer should or can handle.

[maurolacy]

OK, going for that then.

I'll publish these latest changes in a new PR (cosmwasm-crypto package) against main, so that you can take a look at the final version. Besides api / contract testing, and some extra docs / comments, I would say, this is done™️.

Thanks for all the feedback / reviews!

[ethanfrey]

The code looks fine to me. What is missing to me is a use case in a contract.

That can be a different PR, but we need to show/test usage. Having a contract that verifies a signature, and testing it both with unit tests (MockApi) and integration tests (ExternalApi). That, for me, shows it is all working and wired up properly.

[ethanfrey]

Also interesting that this focuses on #751 when I already broke that down into 3 or 4 other tasks, that hopefully could be closed by a PR:

#752
#753
#754
and #756

(I think this may only address #756 as is, but may close them all with a MockApi and tests)

[maurolacy]

What is missing to me is a use case in a contract.

Will do that next, and then work on refactoring stuff (internal cosmwasm-crypto crate).

Update: Also need to add some test cases for the wasm <-> native interface. And some API documentation.

[maurolacy]

Also interesting that this focuses on #751 when I already broke that down into 3 or 4 other tasks...

Yeah, sorry for that. Needed to see it working first, to be able to define the API and do the plumbing.

[maurolacy]

Closing in favour of #779.