Represent signer as a String and use account prefix from the config #711

andynog · 2021-02-24T23:31:01Z

Description

Major refactoring of signer logic to switch it from AccountId to Signer(String). Also includes logic to use the account_prefix from the config to use in the signer account (#635) so the transactions can be submitted to chains that don't use cosmos as the bech32 prefix.

NOTE: Currently there are 3 failing tests that might be related to this changes but I couldn't fix them yet, so just creating a draft PR for now to check if it also passes e2e test.

For contributor use:

Updated the Unreleased section of CHANGELOG.md with the issue.
If applicable: Unit tests written, added test to CI.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Updated relevant documentation (docs/) and code comments.
Re-reviewed Files changed in the Github PR explorer.

romac · 2021-02-25T11:44:10Z

Great stuff @andynog!

While I understand the need to represent the signer as a string rather than a fixed length byte array like AccountId, it would still be nice imho to introduce a wrapper over the String itself to not forfeit some type safety. For example, something like:

#[derive(Clone, Debug, Display, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
pub struct Signer(String);

andynog · 2021-02-25T15:27:47Z

Great stuff @andynog!

While I understand the need to represent the signer as a string rather than a fixed length byte array like AccountId, it would still be nice imho to introduce a wrapper over the String itself to not forfeit some type safety. For example, something like:
#[derive(Clone, Debug, Display, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
pub struct Signer(String);

Thanks @romac, just thinking should we use a [u8] or vec instead of string because the SDK has it as []byte in Go
https://github.com/cosmos/cosmos-sdk/blob/5d3f29b0898bc571bc60a1d53e1e8835f7fb89da/types/address.go#L101

romac · 2021-02-25T16:03:22Z

Right, then yeah something like

#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
pub struct Signer(Vec<u8>);

impl Display for Signer {
  // use bech32 encoding
}

This then gives you to_string() and as_str() to get the bech32-encoded string version.

andynog · 2021-02-26T20:38:27Z

Right, then yeah something like
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
pub struct Signer(Vec<u8>);

impl Display for Signer {
  // use bech32 encoding
}
This then gives you to_string() and as_str() to get the bech32-encoded string version.

@romac I've been looking into this suggestion to wrap the String but the more I look on all the traits that would need to be implemented the more I think it will look like the AccountId from tendermint-rs that we had before except for the LENGTH constraint.

In the end, the signer basically is retrieved from the address property of the KeyEntry and it's a Vec<u8> that is later encoded to bech-32 when get_signer is called. If the String is not in a proper format for bech32 encoding will fail. So not sure if not just leaving it as a string if in the msgs if that should not be OK for now.

ancazamfir · 2021-03-03T16:53:24Z

Trying to follow this, why can't we leave it as String for now? This is how it shows in the raw protocol messages. We should not use any Tendermint AccountId in the ibc crate. Or make any assumptions that we are dealing with an SDK account. All that should be dealt with in the chain impl when we send the messages which are serialized with protobufs so they must be String.
Can still use the wrapper in the domain type messages

#[derive(Clone, Debug, Display, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
pub struct Signer(String);

but it is not clear to me what we gain as we should never look at signer or make any assumptions about it.

modules/src/address.rs

romac · 2021-03-03T21:58:13Z

@andynog @ancazamfir Yeah good points! So let's represent it as a String wrapped in a Signer newtype, which we decode from the byte array in CosmosSdkChain::get_signer.

…_any

codecov-io · 2021-03-10T14:06:48Z

Codecov Report

Merging #711 (fa38d3a) into master (b1b37f5) will increase coverage by 30.5%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    informalsystems/hermes#711      +/-   ##
=========================================
+ Coverage    13.6%   44.1%   +30.5%     
=========================================
  Files          69     156      +87     
  Lines        3752   10288    +6536     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4546    +4033     
- Misses       2618    5742    +3124     
+ Partials      621       0     -621

Impacted Files	Coverage Δ
...application/ics20_fungible_token_transfer/error.rs	`0.0% <ø> (ø)`
...ion/ics20_fungible_token_transfer/msgs/transfer.rs	`0.0% <ø> (ø)`
..._transfer/relay_application_logic/send_transfer.rs	`0.0% <ø> (ø)`
modules/src/events.rs	`0.0% <ø> (ø)`
modules/src/handler.rs	`100.0% <ø> (ø)`
modules/src/ics02_client/client_def.rs	`47.3% <ø> (ø)`
modules/src/ics02_client/client_type.rs	`79.1% <ø> (+31.5%)`	⬆️
modules/src/ics02_client/context.rs	`100.0% <ø> (ø)`
modules/src/ics02_client/error.rs	`100.0% <ø> (ø)`
modules/src/ics02_client/events.rs	`15.7% <ø> (+15.7%)`	⬆️
... and 259 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfd3d8e...fa38d3a. Read the comment docs.

adizere

This is great work, things are a lot cleaner!

I only left one comment worth discussing: the lock file which seems added by accident.

adizere · 2021-03-11T09:07:16Z

modules/src/ics03_connection/msgs/conn_open_try.rs

@@ -30,8 +29,8 @@ pub struct MsgConnectionOpenTry {
    pub counterparty: Counterparty,
    pub counterparty_versions: Vec<Version>,
    pub proofs: Proofs,
-    pub delay_period: u64,
-    pub signer: AccountId,
+    pub delay_period: u64, // FIXME(romac): Introduce newtype for `delay_period`


Yus! Probably the todo should be fixed alongside #640.

adizere · 2021-03-11T09:11:04Z

modules/src/ics04_channel/msgs/recv_packet.rs

@@ -18,22 +18,23 @@ pub const TYPE_URL: &str = "/ibc.core.channel.v1.MsgRecvPacket";
 #[derive(Clone, Debug, PartialEq)]
 pub struct MsgRecvPacket {
    pub packet: Packet,
-    proofs: Proofs,
-    signer: AccountId,
+    pub proofs: Proofs,


@cezarad FYI the diff hunks here. As soon as we merge the present PR, your PR on recv_packet (#737) will need to be adapted.

adizere · 2021-03-11T09:23:28Z

modules/src/tx_msg.rs

-    fn get_signers(&self) -> Vec<AccountId>;
+    fn get_sign_bytes(self) -> Vec<u8> {
+        let mut buf = Vec::new();
+        let raw_msg: Self::Raw = self.into();


Hmm interesting that we're now avoiding a clone here. It used to be let raw_msg: M = self.clone().into();. Perhaps cloning is no longer necessary because we're not using generics anymore?

Later edit: I now see that we're consuming self in the call to to_any and get_sign_bytes, which explains the lack of cloning.

proto-compiler/Cargo.lock

ancazamfir

Looks good! Thanks a lot @andynog and @romac!!

…nformalsystems#711) * Refactoring of signer from AccountId into String (informalsystems#219) * Logic to add the account_prefix from config to signer (informalsystems#219) * Uncomment tests that are failing * Move bech32 encoding of accounts to cosmos chain impl * Use a newtype for the signer * Allow empty signer in tests * Add plain constructor to all ICS04 messages * Remove default implementation of Msg::type_url * Use associated type on Msg trait instead of type parameter on Msg::to_any * Formatting * Remove proto-compiler lockfile Co-authored-by: Romain Ruetschi <romain@informal.systems>

andynog added 4 commits February 23, 2021 18:04

Refactoring of signer from AccountId into String (#219)

f4b9988

Logic to add the account_prefix from config to signer (#219)

4f03218

Uncomment tests that are failing

c97c60d

Merge master (#219)

e4d4c2c

ancazamfir reviewed Mar 3, 2021

View reviewed changes

modules/src/address.rs Outdated Show resolved Hide resolved

romac changed the title ~~Andy/acct prefix~~ Represent signer as a String and use account prefix from the config Mar 4, 2021

romac added 9 commits March 4, 2021 16:10

Merge branch 'master' into andy/acct_prefix

d40abd4

Move bech32 encoding of accounts to cosmos chain impl

d242510

Use a newtype for the signer

9da5d96

Allow empty signer in tests

5eefdc6

Add plain constructor to all ICS04 messages

d3e70ba

Remove default implementation of Msg::type_url

d752efe

Use associated type on Msg trait instead of type parameter on Msg::to…

d9c47a5

…_any

Formatting

c325894

Merge branch 'master' into andy/acct_prefix

fa38d3a

romac marked this pull request as ready for review March 10, 2021 14:30

romac self-requested a review as a code owner March 10, 2021 14:30

adizere reviewed Mar 11, 2021

View reviewed changes

Remove proto-compiler lockfile

397d0cd

romac requested a review from adizere March 11, 2021 10:02

ancazamfir approved these changes Mar 11, 2021

View reviewed changes

adizere approved these changes Mar 11, 2021

View reviewed changes

adizere merged commit 9c0c5d6 into master Mar 11, 2021

adizere deleted the andy/acct_prefix branch March 11, 2021 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Represent signer as a String and use account prefix from the config #711

Represent signer as a String and use account prefix from the config #711

andynog commented Feb 24, 2021 •

edited by romac

Loading

romac commented Feb 25, 2021 •

edited

Loading

andynog commented Feb 25, 2021

romac commented Feb 25, 2021 •

edited

Loading

andynog commented Feb 26, 2021 •

edited

Loading

ancazamfir commented Mar 3, 2021 •

edited

Loading

romac commented Mar 3, 2021 •

edited

Loading

codecov-io commented Mar 10, 2021

adizere left a comment

adizere Mar 11, 2021

adizere Mar 11, 2021

adizere Mar 11, 2021

ancazamfir left a comment

Represent signer as a String and use account prefix from the config #711

Represent signer as a String and use account prefix from the config #711

Conversation

andynog commented Feb 24, 2021 • edited by romac Loading

Description

romac commented Feb 25, 2021 • edited Loading

andynog commented Feb 25, 2021

romac commented Feb 25, 2021 • edited Loading

andynog commented Feb 26, 2021 • edited Loading

ancazamfir commented Mar 3, 2021 • edited Loading

romac commented Mar 3, 2021 • edited Loading

codecov-io commented Mar 10, 2021

Codecov Report

adizere left a comment

Choose a reason for hiding this comment

adizere Mar 11, 2021

Choose a reason for hiding this comment

adizere Mar 11, 2021

Choose a reason for hiding this comment

adizere Mar 11, 2021

Choose a reason for hiding this comment

ancazamfir left a comment

Choose a reason for hiding this comment

andynog commented Feb 24, 2021 •

edited by romac

Loading

romac commented Feb 25, 2021 •

edited

Loading

romac commented Feb 25, 2021 •

edited

Loading

andynog commented Feb 26, 2021 •

edited

Loading

ancazamfir commented Mar 3, 2021 •

edited

Loading

romac commented Mar 3, 2021 •

edited

Loading