wormhole-foundation / wormhole

A reference implementation for the Wormhole blockchain interoperability protocol.
https://wormhole.com
Other
1.68k stars 690 forks source link

Add bindings for cosmwasm contracts to query the native cosmos wormhole module #1881

Closed jynnantonix closed 1 year ago

jynnantonix commented 2 years ago

Proposed API:

#[cw_serde]
pub struct Signature {
    // The index of the observer in the observer set.
    pub index: u16,

    // The signature.
    pub signature: Binary,
}

#[cw_serde]
#[derive(QueryResponses)]
pub enum WormholeQuery {
    // Verifies that `data` has been signed by a quorum of observers from `observer_set_index`.
    #[returns(Empty)]
    VerifyQuorum {
        data: Binary,
        guardian_set_index: u32,
        signatures: Vec<Signature>,
    },

    // Verifies that `data` has been signed by an observer from `observer_set_index`.
    #[returns(Empty)]
    VerifySignature {
        data: Binary,
        guardian_set_index: u32,
        signature: Signature,
    },

    // Returns the number of signatures necessary for quorum for the given observer set index.
    #[returns(u32)]
    CalculateQuorum { guardian_set_index: u32 },
}
jynnantonix commented 2 years ago

@kcsongor @hendrikhofstadt @conorpp for API review

kcsongor commented 1 year ago

Overall looks good, what does the block_time mean in the VerifySignature query?

jynnantonix commented 1 year ago

block_time is the value of env.block.height. Maybe it would be better if it was renamed to block_height?

kcsongor commented 1 year ago

block_height sounds a little better. Is the idea there to verify if a VAA was valid at a given time in the past (to account for guardian set upgrades)?

jynnantonix commented 1 year ago

The block height is used to check if the guardian set has expired. If there's a way for the native module to independently fetch the current block height while handling this query then we can remove this field completely.

kcsongor commented 1 year ago

The handler in the native module will have access to the Context struct https://github.com/wormhole-foundation/cosmos-sdk/blob/9f2b91495bcea23d427170597b4136673008cb56/types/context.go#L25-L42 which in turn contains the BlockHeight https://github.com/wormhole-foundation/cosmos-sdk/blob/9f2b91495bcea23d427170597b4136673008cb56/types/context.go#L50

conorpp commented 1 year ago

This looks okay to me.

I think another approach would be to query just the guardian sets and calculate quorum and verifications locally. Reimplementing this logic is risky, but I believe we already have audited/tested implementations for it in cosmwasm from the Terra token bridge contract? Basically just import the guardian sets into that implementation then do similar checks. I'm not 100% sure if we'd ever need to extend/change this API, but if we had the guardian set information we could extend it without needing a wormchain update.

jynnantonix commented 1 year ago

I think another approach would be to query just the guardian sets and calculate quorum and verifications locally.

I thought about this but I figured doing signature verification in wasm would be a lot slower than doing it in native code. Since we have to send a query to fetch the guardian set anyway, it seemed better to have the native module just handle everything.

jynnantonix commented 1 year ago

I've updated the bindings in #1917 to drop the block_time field completely since the query handler should be able to fetch the block height independently.

jynnantonix commented 1 year ago

This was fixed by #2108