Refactor send_tx methods into plain functions
#2044
Conversation
| use crate::error::Error; | ||
| use crate::keyring::KeyEntry; | ||
|
|
||
| pub async fn send_batched_messages_and_wait_commit( |
There was a problem hiding this comment.
This is refactored from send_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>, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Let's address this in a follow-up PR, perhaps even in #2078.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
| @@ -7,6 +7,7 @@ | |||
| unused_qualifications, | |||
| rust_2018_idioms | |||
| )] | |||
| #![allow(clippy::too_many_arguments)] | |||
There was a problem hiding this comment.
@mzabaluev the lint is overridden here
This PR attempts to rework the core logic of `send_tx` methods in `CosmosSdkChain` into plain functions. This is essential in helping us to solve the root problems in issues such as informalsystems#2012, informalsystems#2040, and supporting sending of transactions with fees in ICS29. At this stage, I have managed to refactor `send_tx_with_account_sequence_retry` and its inner calls into plain functions, and simplify the data access a little. Since this is a refactoring PR, no logic should be changed. The integration tests should help to ensure that nothing breaks with the refactoring. --- * Refactoring send_tx * WIP refactoring * More code reorganization * Split out functions from main cosmos.rs * Use refactored code to send_tx * Walk through send_tx_with_account_sequence * Refactor code * Reorder function arguments * Refactor send_tx_with_account_sequence_retry into plain function * Refactor account query functions * Refactor query_tx * Refactor wait_for_block_commits * Turn wait_for_block_commits into simple loop * Refactor send_messages_and_wait_commit * Refactor send_messages_and_wait_check_tx * Refactor sign_message * Refactor gas config * Move out query account module * Reorganize types * Remove pub const * Simplify arguments * Remove redundant account query function * Refactor query status * Introduce TransferTimeout abstraction * Use prost::Message::encoded_len() to compute encoded message length * Address review feedback * Re-add missing comments * Fix clippy error * Remove check for both timeout height offset and duration being zero * Do not set timeout height or time when input is zero
Closes #1975.
Description
This PR attempts to rework the core logic of
send_txmethods inCosmosSdkChaininto plain functions. This is essential in helping us to solve the root problems in issues such as #2012, #2040, and supporting sending of transactions with fees in ICS29.At this stage, I have managed to refactor
send_tx_with_account_sequence_retryand its inner calls into plain functions, and simplify the data access a little.Since this is a refactoring PR, no logic should be changed. The integration tests should help to ensure that nothing breaks with the refactoring.
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.