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

Refactor send_tx methods into plain functions #2044

Merged
merged 39 commits into from
Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
162fea4
Refactoring send_tx
soareschen Mar 11, 2022
d73a8b8
WIP refactoring
soareschen Mar 14, 2022
586dadc
Merge branch 'master' into soares/refactor-send-tx
soareschen Mar 18, 2022
517432f
More code reorganization
soareschen Mar 18, 2022
8d4d371
Split out functions from main cosmos.rs
soareschen Mar 18, 2022
b7ff345
Use refactored code to send_tx
soareschen Mar 18, 2022
9333179
Merge branch 'master' into soares/refactor-send-tx
soareschen Mar 31, 2022
f24fd1a
Walk through send_tx_with_account_sequence
soareschen Apr 1, 2022
bb989f8
Refactor code
soareschen Apr 1, 2022
a9eec75
Reorder function arguments
soareschen Apr 1, 2022
0b3f89c
Refactor send_tx_with_account_sequence_retry into plain function
soareschen Apr 1, 2022
4aac7c8
Refactor account query functions
soareschen Apr 4, 2022
4d791fa
Refactor query_tx
soareschen Apr 4, 2022
65f9de6
Refactor wait_for_block_commits
soareschen Apr 4, 2022
80ab989
Turn wait_for_block_commits into simple loop
soareschen Apr 4, 2022
732b050
Merge remote-tracking branch 'origin/master' into soares/refactor-sen…
soareschen Apr 4, 2022
a84057e
Refactor send_messages_and_wait_commit
soareschen Apr 5, 2022
6f1596a
Refactor send_messages_and_wait_check_tx
soareschen Apr 5, 2022
7bdf017
Merge remote-tracking branch 'origin/master' into soares/refactor-sen…
soareschen Apr 5, 2022
a0ef196
Refactor sign_message
soareschen Apr 5, 2022
9262f00
Refactor gas config
soareschen Apr 5, 2022
5dad18c
Move out query account module
soareschen Apr 5, 2022
8d1040c
Reorganize types
soareschen Apr 5, 2022
646da40
Remove pub const
soareschen Apr 5, 2022
2dae995
Simplify arguments
soareschen Apr 5, 2022
7fe9488
Remove redundant account query function
soareschen Apr 5, 2022
206dfc2
Refactor query status
soareschen Apr 6, 2022
7f25765
Introduce TransferTimeout abstraction
soareschen Apr 6, 2022
d245cdb
Use prost::Message::encoded_len() to compute encoded message length
soareschen Apr 7, 2022
ad4deca
Merge remote-tracking branch 'origin/master' into soares/refactor-sen…
soareschen Apr 7, 2022
52992f6
Address review feedback
soareschen Apr 7, 2022
2d101f3
Re-add missing comments
soareschen Apr 7, 2022
e621b07
Merge remote-tracking branch 'origin/master' into soares/refactor-sen…
soareschen Apr 8, 2022
17ade0d
Fix clippy error
soareschen Apr 8, 2022
c5a88b1
Merge remote-tracking branch 'origin/master' into soares/refactor-sen…
soareschen Apr 11, 2022
073b057
Remove check for both timeout height offset and duration being zero
soareschen Apr 11, 2022
225f241
Do not set timeout height or time when input is zero
soareschen Apr 12, 2022
2f64495
Merge remote-tracking branch 'origin/master' into soares/refactor-sen…
soareschen Apr 12, 2022
0580999
Merge branch 'master' into soares/refactor-send-tx
soareschen Apr 12, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,248 changes: 123 additions & 1,125 deletions relayer/src/chain/cosmos.rs

Large diffs are not rendered by default.

108 changes: 0 additions & 108 deletions relayer/src/chain/cosmos/account.rs

This file was deleted.

174 changes: 174 additions & 0 deletions relayer/src/chain/cosmos/batch.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
use ibc::events::IbcEvent;
use ibc_proto::google::protobuf::Any;
use tendermint_rpc::endpoint::broadcast::tx_sync::Response;
use tendermint_rpc::{HttpClient, Url};
use tonic::codegen::http::Uri;

use crate::chain::cosmos::retry::send_tx_with_account_sequence_retry;
use crate::chain::cosmos::types::account::Account;
use crate::chain::cosmos::types::tx::TxSyncResult;
use crate::chain::cosmos::wait::wait_for_block_commits;
use crate::config::types::Memo;
use crate::config::ChainConfig;
use crate::error::Error;
use crate::keyring::KeyEntry;

pub async fn send_batched_messages_and_wait_commit(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

config: &ChainConfig,
rpc_client: &HttpClient,
rpc_address: &Url,
grpc_address: &Uri,
key_entry: &KeyEntry,
account: &mut Account,
tx_memo: &Memo,
messages: Vec<Any>,
Comment on lines +18 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

This turns a two-argument function into an eight(!)-argument function.
There is a clippy lint against this, is it overridden somewhere?

Can this be alleviated with struct arguments with largely Default-initialized fields, to simplify use of these functions? Something in the spirit of #2078 would be nice.

Copy link
Member

Choose a reason for hiding this comment

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

Related to #375

Copy link
Member

Choose a reason for hiding this comment

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

Let's address this in a follow-up PR, perhaps even in #2078.

Copy link
Contributor Author

@soareschen soareschen Apr 13, 2022

Choose a reason for hiding this comment

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

I can go on a whole day to argue on this. But for time sake, the short summary is that having many function arguments reveals the symptom of a program design. The practice of hiding many arguments behind an object the OOP way is merely covering up the symptom, and allow the program design to further devolve because it does not look that bad syntactically when new fields are added.

The right way to fix the program design is to identify the symptoms of a program design by revealing how many variables a piece of code depends on, and then refactor on it. A functional design allows the program to be refactored incrementally, i.e. we can reduce the arguments passed into one function from the bottom up without having to touch the other functions. On the other hand, an OOP approach makes it much more challenging to perform any refactoring, because modifying one object field unavoidably affect every method in the monolithic object.

Here we have surfaced up the symptoms of the original program design and shows what need to be fixed next. In future PRs we can then identify proper functional design patterns that can simplify the code by curing the symptoms, instead of hiding the symptoms behind OOP again.

Copy link
Contributor Author

@soareschen soareschen Apr 13, 2022

Choose a reason for hiding this comment

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

Also note that unlike the original OOP methods, the function arguments do not propagate uniformly to all inner functions. There are also difference in reference types, with there being only one &mut reference as compared to the viral &mut self that allows any variable to be modified.

The OOP design is especially unsuited for Rust when &mut references are involved. It overly broadens every field in an object to become mutable, and the mutation infection easily spread until it turns every object method into using &mut. Mutable objects are especially difficult to deal with in Rust because of the exclusive reference. Before we know it, we get a monolithic object hiding behind Arc<Mutex> because the use of &mut self went out of control.

Copy link
Contributor

@mzabaluev mzabaluev Apr 13, 2022

Choose a reason for hiding this comment

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

I agree the "god object" pattern perpetuated, in part, by ChainEndpoint must be refactored. I'm not sure making the code less usable temporarily is a good way to go about it, but I'm withholding my objection for the sake of expediency and a start on better code organization.

A vaguely related example where a proliferation of flat, non-composable functions makes the code less maintainable is the various build_*{,_and_send} methods for each type of message, with largely duplicated implementation code.

) -> Result<Vec<IbcEvent>, Error> {
if messages.is_empty() {
return Ok(Vec::new());
}

let mut tx_sync_results = send_messages_as_batches(
config,
rpc_client,
grpc_address,
key_entry,
account,
tx_memo,
messages,
)
.await?;

wait_for_block_commits(
&config.id,
rpc_client,
rpc_address,
&config.rpc_timeout,
&mut tx_sync_results,
)
.await?;

let events = tx_sync_results
.into_iter()
.flat_map(|el| el.events)
.collect();

Ok(events)
}

pub async fn send_batched_messages_and_wait_check_tx(
soareschen marked this conversation as resolved.
Show resolved Hide resolved
config: &ChainConfig,
rpc_client: &HttpClient,
grpc_address: &Uri,
key_entry: &KeyEntry,
account: &mut Account,
tx_memo: &Memo,
messages: Vec<Any>,
) -> Result<Vec<Response>, Error> {
if messages.is_empty() {
return Ok(Vec::new());
}

let batches = batch_messages(config, messages)?;

let mut responses = Vec::new();

for batch in batches {
let response = send_tx_with_account_sequence_retry(
config,
rpc_client,
grpc_address,
key_entry,
account,
tx_memo,
batch,
0,
)
.await?;

responses.push(response);
}

Ok(responses)
}

async fn send_messages_as_batches(
soareschen marked this conversation as resolved.
Show resolved Hide resolved
config: &ChainConfig,
rpc_client: &HttpClient,
grpc_address: &Uri,
key_entry: &KeyEntry,
account: &mut Account,
tx_memo: &Memo,
messages: Vec<Any>,
) -> Result<Vec<TxSyncResult>, Error> {
if messages.is_empty() {
return Ok(Vec::new());
}

let batches = batch_messages(config, messages)?;

let mut tx_sync_results = Vec::new();

for batch in batches {
let events_per_tx = vec![IbcEvent::default(); batch.len()];

let response = send_tx_with_account_sequence_retry(
config,
rpc_client,
grpc_address,
key_entry,
account,
tx_memo,
batch,
0,
)
.await?;

let tx_sync_result = TxSyncResult {
response,
events: events_per_tx,
};

tx_sync_results.push(tx_sync_result);
}

Ok(tx_sync_results)
}

fn batch_messages(config: &ChainConfig, messages: Vec<Any>) -> Result<Vec<Vec<Any>>, Error> {
soareschen marked this conversation as resolved.
Show resolved Hide resolved
let max_message_count = config.max_msg_num.0;
let max_tx_size = config.max_tx_size.into();

let mut batches = vec![];

let mut current_count = 0;
let mut current_size = 0;
let mut current_batch = vec![];

for message in messages.into_iter() {
current_count += 1;
current_size += message_size(&message)?;
current_batch.push(message);

if current_count >= max_message_count || current_size >= max_tx_size {
let insert_batch = current_batch.drain(..).collect();
batches.push(insert_batch);
current_count = 0;
current_size = 0;
}
}

if !current_batch.is_empty() {
batches.push(current_batch);
}

Ok(batches)
}

fn message_size(message: &Any) -> Result<usize, Error> {
soareschen marked this conversation as resolved.
Show resolved Hide resolved
let mut buf = Vec::new();

prost::Message::encode(message, &mut buf)
.map_err(|e| Error::protobuf_encode("Message".into(), e))?;

Ok(buf.len())
}
Loading