-
Notifications
You must be signed in to change notification settings - Fork 326
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
Changes from 25 commits
162fea4
d73a8b8
586dadc
517432f
8d4d371
b7ff345
9333179
f24fd1a
bb989f8
a9eec75
0b3f89c
4aac7c8
4d791fa
65f9de6
80ab989
732b050
a84057e
6f1596a
7bdf017
a0ef196
9262f00
5dad18c
8d1040c
646da40
2dae995
7fe9488
206dfc2
7f25765
d245cdb
ad4deca
52992f6
2d101f3
e621b07
17ade0d
c5a88b1
073b057
225f241
2f64495
0580999
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
This file was deleted.
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( | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This turns a two-argument function into an eight(!)-argument function. Can this be alleviated with struct arguments with largely There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to #375 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The OOP design is especially unsuited for Rust when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree the "god object" pattern perpetuated, in part, by A vaguely related example where a proliferation of flat, non-composable functions makes the code less maintainable is the various |
||
) -> 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()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is refactored from
send_messages_and_wait_commit
:https://github.com/informalsystems/ibc-rs/blob/3e34b4b916eb91f680412e5fdd3ea045a99526d2/relayer/src/chain/cosmos.rs#L1068-L1071