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

Allow instantiating relayer with different wallets in integration test #2040

Open
5 tasks
soareschen opened this issue Mar 31, 2022 · 0 comments
Open
5 tasks
Assignees
Labels
A: blocked Admin: blocked by another (internal/external) issue or PR O: tests Objective: Test more aspect of the relayer
Milestone

Comments

@soareschen
Copy link
Contributor

soareschen commented Mar 31, 2022

Summary

Make it simple to spawn relayer supervisor and ChainHandles with different wallet provided in the config.

Problem Definition

A key challenge with the current design of ChainHandle is that the wallet is configured pseudo-globally and cannot be changed dynamically or overridden when a transaction is submitted. This present challenges when writing integration tests, because if the same wallet is used concurrently in different operations, the account sequence mismatch errors will happen and cause the tests to potentially fail.

This presents problem if we want to spawn multiple relayers to run in parallel, as in the case of #2024. To do so, we need to have ways to instantiate multiple relayer configurations with different wallets, and specify which relayer or ChainHandle we want to choose from.

We also have similar problem of sending transactions with custom wallets which is described in #1975.

One complication for the design is that we cannot easily swap the wallets within a relayer's Config value. This is because the wallets are different for each chain, so we have to effectively change a whole set of wallets depending on how many chains are present in the config.

Proposal

#2039 is a work in progress to figure out how we can solve this problem without too much boilerplate. It uses const generics with a hardcoded array of relayer wallets, so that tests can for example get 3 sets of relayers to be used.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@soareschen soareschen added the O: tests Objective: Test more aspect of the relayer label Mar 31, 2022
@romac romac linked a pull request Mar 31, 2022 that will close this issue
6 tasks
@adizere adizere added this to the v0.14.0 milestone Apr 5, 2022
@adizere adizere added the A: blocked Admin: blocked by another (internal/external) issue or PR label Apr 5, 2022
romac pushed a commit that referenced this issue Apr 13, 2022
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 #2012, #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
@adizere adizere modified the milestones: v0.14.0, v0.15.0, v2 Apr 25, 2022
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this issue Sep 13, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: blocked Admin: blocked by another (internal/external) issue or PR O: tests Objective: Test more aspect of the relayer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants