Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GRPC refactor #2905

Merged
merged 14 commits into from
Nov 10, 2023
Merged

Conversation

tkporter
Copy link
Collaborator

@tkporter tkporter commented Nov 9, 2023

Description

  • Creates a single channel that's cloned (which can be done cheaply and is encouraged) when creating a new client.
  • general cleanup & renaming
  • I believe a chain signer will not be required by e.g. the validator, will confirm this locally

Drive-by changes

n/a

Related issues

n/a

Backward compatibility

yee

Testing

builds, local test

@tkporter tkporter requested a review from daniel-savu as a code owner November 9, 2023 11:43
Copy link
Contributor

@daniel-savu daniel-savu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master grpc optimizor 🏎️

rust/chains/hyperlane-cosmos/src/mailbox.rs Outdated Show resolved Hide resolved
Comment on lines +48 to +51
/// Note that in Tendermint, validators come to consensus on a block
/// before they execute the transactions in that block. This means that
/// we may not be able to make state queries against this block until
/// the next one is committed!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much needed comments 🙏

// As this function is only used for estimating gas or sending transactions,
// we can reasonably expect to have a signer.
let signer = self.get_signer()?;
let account_info = self.account_query(signer.address.clone()).await?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we're not precomputing this in the constructor because we want to keep it synchronous? Don't think it's worth caching otherwise but was just wondering

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "account number" can be cached but fetching the sequence number (i.e. the nonce to use for the next tx) should still be done. Just in case other txs from that key happen to have been made elsewhere


let tx_body = tx::Body::new(
msgs,
"",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleaner to use Default::default() here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the expected value is impl Into<String> so I'll just do String::default()

let raw_tx = TxRaw {
body_bytes: sign_doc.body_bytes,
auth_info_bytes: sign_doc.auth_info_bytes,
// The poorly documented trick to simuluating a tx without a valid signature is to just pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@tkporter tkporter merged commit a95d424 into trevor/new-featv3-cosmos-oct-28 Nov 10, 2023
2 of 4 checks passed
@tkporter tkporter deleted the trevor/grpc-refactor-1 branch November 10, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants