cosmos-tx: MsgSend support w\ integration test #71

tony-iqlusion · 2021-04-06T01:28:16Z

Basic support for MsgSend with end-to-end integration test.

Adds the following:

Traits for simplifying Protobuf serialization:
- prost_ext::MessageExt for Protobuf encoding
- msg::MsgType and msg::MsgProto for decoding/encoding Msg types as Protobuf Any messages.
Domain types which model the following:
- Coin: amount to send and a denom
- Denom: name of a particular denomination
- Fee: transaction fees
- MsgSend: transaction message for performing a simple send

Additionally includes an end-to-end test which uses Docker to spawn a single-node gaia and send a transaction.

Note: at least as of the current state of this draft, the end-to-end test is not run as part of CI, but that seems possible.

tony-iqlusion · 2021-04-06T01:31:45Z

This isn't quite working but it's getting pretty far. It's able to build a MsgSend transaction, spawn a single-node gaia network running under the jackzampolin/gaiatest Docker image, and uses tendermint-rpc to submit a transaction.

The transaction is failing validation due to a parse error, but that should be fairly trivial to fix hopefully. The good news is most of the moving pieces are talking to each other and we're down to the nitty gritty:

Response {
    check_tx: TxResult {
        code: Err(
            2,
        ),
        data: None,
        log: Log(
            "math/big: cannot unmarshal \"1000000.000000000000000000\" into a *big.Int: tx parse error",
        ),
        info: Info(
            "",
        ),
        gas_wanted: Gas(
            0,
        ),
        gas_used: Gas(
            0,
        ),
        events: [],
        codespace: Codespace(
            "sdk",
        ),
    },
    deliver_tx: TxResult {
        code: Ok,
        data: None,
        log: Log(
            "",
        ),
        info: Info(
            "",
        ),
        gas_wanted: Gas(
            0,
        ),
        gas_used: Gas(
            0,
        ),
        events: [],
        codespace: Codespace(
            "",
        ),
    },
    hash: transaction::Hash(1BC3F3638A43BB02766D48772D2771B29458056F2755692F369B3E34E2834124),
    height: block::Height(0),
}

tony-iqlusion · 2021-04-06T02:02:45Z

One other thing I should mention:

Everything I've added in this PR is attempting to be isomorphic to cosmos-sdk, including the module structure and type names.

It seems the scope of types is actually pretty vast even to implement a simple send, and the more I think about it, the more this crate should probably be cosmos-sdk (a.k.a. "cosmos-sdk-rs" to disambiguate from the Go library, in the spirit of "tendermint-rs").

I think it'd be nice to get rid of cosmos_tx::Builder and replace it with a cosmos_tx or cosmos_sdk::tx::TxBody domain type, which can be signed/serialized or deserialized from protos, more or less following a similar approach to what tendermint-rs is doing.

I'm avoiding making that particular change in this PR in order to make it easier to review, but I think it'd be a nice followup refactoring which would, at the same time, add deserialization support for transactions.

cosmos-tx/src/msg.rs

cosmos-tx/tests/integration.rs

jkilpatr · 2021-04-07T15:27:58Z

Nice work @tony-iqlusion

While you where doing this I've pulled your cosmos-tx crate into Althea's deep_space library, mostly for the same purposes.

https://github.com/althea-net/deep_space

I'm currently testing out proto tx from that codebase now. The reason I pulled code down rather than up-streaming is mostly because deep_space has a much larger scope. As a grpc library, with basic key management and mnemonic parsing and a whole pile of utility functions in addition tx signing.

Right now I have a lot of work to do to reconcile the different secp256 crates, I do some manual bit hacking for the key import and generation code that doesn't play so well with the pure rust version, which I'd rather be using in general.

I think it's worth discussing what a cosmos-sdk crate would look like. Should we be thinking of a batteries included library or series of libraries? how should they be broken out. Etc.

In terms of simple action items, up-streaming deep_space's simple key management and mnemonic parsing and then the grpc library as seperate components could be worthwhile. Or we could upstream into a more monolithic library. Very flexible key management will be the biggest challenge I believe.

tony-iqlusion · 2021-04-07T18:13:56Z

For what it's worth I've been collaborating with the ethers-rs developers on a BIP32 crate (also supporting BIP39) which is based on k256:

iqlusioninc/crates#670

tony-iqlusion · 2021-04-09T02:33:59Z

Just pushed some changes up that are sufficient to get the signature to verify as part of a local integration test.

I'll do some additional cleanups and see if I can get the full integration test to run as part of GitHub Actions and then remove draft/WIP (hopefully tomorrow)

Basic support for `MsgSend` with end-to-end integration test. Adds the following: - Traits for simplifying Protobuf serialization: - `prost_ext::MessageExt` for Protobuf encoding - `msg::MsgType` and `msg::MsgProto` for decoding/encoding `Msg` types as Protobuf `Any` messages. - Domain types which model the following: - `Body`: body of a transaction - `Coin`: amount to send and a denom - `Denom`: name of a particular denomination - `Fee`: transaction fees - `MsgSend`: transaction message for performing a simple send Additionally includes an end-to-end test which uses Docker to spawn a single-node `gaia` and send a transaction.

tony-iqlusion · 2021-04-09T18:03:47Z

Updated the PR so it runs the gaia container as part of the CI workflow and the tests are passing! 🎉

Removed draft/WIP.

jkilpatr · 2021-04-09T18:18:06Z

cosmos-tx/src/base.rs

+    hrp_length: usize,
+}
+
+impl AccountId {


big thumbs on on local types for these things. 👍🏻

jkilpatr · 2021-04-09T18:20:44Z

cosmos-tx/src/lib.rs

+pub use k256::ecdsa::Signature;
+pub use tendermint;
+
+#[cfg(feature = "rpc")]


I'm on the fence about the usefulness of an RPC feature, since we're pulling in Tonic as a transitive dep from cosmos-sdk-proto the only advantage an RPC feature gives is keeping us from using any tonic calls and then hoping LTO removes tonic from the final binary.

We should be able to get the same results by just not using RPC calls in a final binary, unless I'm missing somthing.

Thoughts?

We need it for the tests anyway, so it can either exist as test-specific functionality or a reusable part of the public API. I opted for the latter.

It's an off-by-default optional feature, so it doesn't seem problematic to me for users who aren't interested in it.

Def agree that it should be a part of the public API. I just don't think feature gating it is useful if we don't reduce the dependency tree by doing so.

I don't have very strong feelings about this, just a comment.

cosmos-sdk-proto is being pulled in with default-features = false, so it isn't pulling in Tonic:

https://github.com/cosmos/cosmos-rust/blob/main/cosmos-tx/Cargo.toml#L14

jkilpatr · 2021-04-09T18:23:00Z

cosmos-tx/tests/integration.rs

+                panic!("deliver_tx failed: {:?}", tx_commit_response.deliver_tx);
+            }
+
+            // TODO(tarcieri): look up transaction by hash and test transaction parsing


I have the lookup grpc code, should I just upstream that as a broken out cate? I can integrate it in here.

Since we already have an RPC client instantiated, I can use the /tx_search endpoint to look it up by hash:

https://docs.rs/tendermint-rpc/0.19.0/tendermint_rpc/trait.Client.html#method.tx_search

jkilpatr · 2021-04-09T18:23:13Z

cosmos-tx/tests/integration.rs

+/// if the command completed successfully.
+///
+/// Panics if the `docker` process exits with an error code
+fn exec_docker_command<A, S>(name: &str, args: A) -> String


this is pretty cool.

I was thinking of extracting this functionality in such a way it's reusable, e.g. behind a dev feature.

I think it'd be useful for testing TMKMS transaction signing.

But I thought I'd save that for a follow-up PR.

cosmos-tx/tests/integration.rs

tony-iqlusion · 2021-04-09T18:35:43Z

cosmos-tx/tests/integration.rs

+                panic!("deliver_tx failed: {:?}", tx_commit_response.deliver_tx);
+            }
+
+            // TODO(tarcieri): look up transaction by hash and test transaction parsing


I'd like to do this, but this PR is already so large I thought it was worth skipping for now and doing it in a follow-up.

tony-iqlusion force-pushed the tony-iqlusion/cosmos-tx-msg-send-and-integration-test branch 2 times, most recently from cd49f82 to fc4aff5 Compare April 6, 2021 01:37

tony-iqlusion force-pushed the tony-iqlusion/cosmos-tx-msg-send-and-integration-test branch 2 times, most recently from 3f30113 to cc5a5c5 Compare April 6, 2021 03:31

tony-iqlusion commented Apr 6, 2021

View reviewed changes

cosmos-tx/src/msg.rs Outdated Show resolved Hide resolved

tony-iqlusion commented Apr 6, 2021

View reviewed changes

cosmos-tx/tests/integration.rs Outdated Show resolved Hide resolved

tony-iqlusion force-pushed the tony-iqlusion/cosmos-tx-msg-send-and-integration-test branch 10 times, most recently from fbcd9e8 to 9558257 Compare April 6, 2021 21:35

jkilpatr self-requested a review April 7, 2021 15:20

tony-iqlusion force-pushed the tony-iqlusion/cosmos-tx-msg-send-and-integration-test branch 8 times, most recently from 44a6e8d to 7fd5071 Compare April 9, 2021 02:30

tony-iqlusion force-pushed the tony-iqlusion/cosmos-tx-msg-send-and-integration-test branch 2 times, most recently from 9c24e70 to 8fc3444 Compare April 9, 2021 17:49

tony-iqlusion changed the title ~~[WIP] cosmos-tx: MsgSend support w\ integration test~~ cosmos-tx: MsgSend support w\ integration test Apr 9, 2021

tony-iqlusion force-pushed the tony-iqlusion/cosmos-tx-msg-send-and-integration-test branch from 8fc3444 to b572c0a Compare April 9, 2021 17:54

tony-iqlusion force-pushed the tony-iqlusion/cosmos-tx-msg-send-and-integration-test branch from b572c0a to 210f33b Compare April 9, 2021 17:57

tony-iqlusion marked this pull request as ready for review April 9, 2021 18:03

tony-iqlusion requested a review from jackzampolin April 9, 2021 18:03

jkilpatr approved these changes Apr 9, 2021

View reviewed changes

tony-iqlusion commented Apr 9, 2021

View reviewed changes

cosmos-tx/tests/integration.rs Outdated Show resolved Hide resolved

tony-iqlusion commented Apr 9, 2021

View reviewed changes

CI: have integration test wait for first block to be produced

a07a212

tony-iqlusion merged commit 386d7e0 into main Apr 9, 2021

tony-iqlusion deleted the tony-iqlusion/cosmos-tx-msg-send-and-integration-test branch April 9, 2021 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cosmos-tx: MsgSend support w\ integration test #71

cosmos-tx: MsgSend support w\ integration test #71

tony-iqlusion commented Apr 6, 2021 •

edited

Loading

tony-iqlusion commented Apr 6, 2021

tony-iqlusion commented Apr 6, 2021 •

edited

Loading

jkilpatr commented Apr 7, 2021 •

edited

Loading

tony-iqlusion commented Apr 7, 2021

tony-iqlusion commented Apr 9, 2021

tony-iqlusion commented Apr 9, 2021

jkilpatr Apr 9, 2021

jkilpatr Apr 9, 2021

tony-iqlusion Apr 9, 2021

jkilpatr Apr 9, 2021

tony-iqlusion Apr 9, 2021

jkilpatr Apr 9, 2021

tony-iqlusion Apr 9, 2021

jkilpatr Apr 9, 2021

tony-iqlusion Apr 9, 2021

tony-iqlusion Apr 9, 2021

cosmos-tx: MsgSend support w\ integration test #71

cosmos-tx: MsgSend support w\ integration test #71

Conversation

tony-iqlusion commented Apr 6, 2021 • edited Loading

tony-iqlusion commented Apr 6, 2021

tony-iqlusion commented Apr 6, 2021 • edited Loading

jkilpatr commented Apr 7, 2021 • edited Loading

tony-iqlusion commented Apr 7, 2021

tony-iqlusion commented Apr 9, 2021

tony-iqlusion commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony-iqlusion commented Apr 6, 2021 •

edited

Loading

tony-iqlusion commented Apr 6, 2021 •

edited

Loading

jkilpatr commented Apr 7, 2021 •

edited

Loading