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

ChainDriver::transfer_token() does not perform subsequent IBC transfer when called multiple times #2012

Closed
5 tasks
soareschen opened this issue Mar 25, 2022 · 1 comment · Fixed by #2205
Closed
5 tasks
Assignees
Labels
A: bug Admin: something isn't working A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews O: tests Objective: Test more aspect of the relayer
Milestone

Comments

@soareschen
Copy link
Contributor

Summary of Bug

Currently the ChainDriver::transfer_token() method calls the Gaia command gaiad tx ibc-transfer transfer to perform the IBC transfer. However when submitting the IBC transfer one right after another, only the first call of it would pass.

This might be a limitation of gaiad that we have to investigate. Otherwise, we would have to reimplement ChainDriver::transfer_token() to perform the IBC transfer using the ChainHandle method.

Steps to Reproduce

#2011 modifies the IBC transfer test to perform multiple IBC transfers at once. The result is that the test fails unless TRANSFER_COUNT is set to 1.

Acceptance Criteria

Multiple calls to ChainDriver::transfer_token() should succeed in submitting IBC transfer.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@soareschen soareschen added O: tests Objective: Test more aspect of the relayer A: bug Admin: something isn't working labels Mar 25, 2022
@soareschen
Copy link
Contributor Author

A temporary workaround in #2024 is to wait for 1 second before performing another transfer_token(). This is probably due to gaiad only supporting a single transaction to be submitted per wallet per block. So consecutive calls within the same block period will fail.

An alternative workaround is to poll the sender wallet balance before and after performing the transaction, and only return after the wallet balance has been reduced. This will require the least amount of work as compared to other kinds of refactoring.

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 added this to the v0.15.0 milestone Apr 25, 2022
@adizere adizere moved this to Backlog in IBC-rs: the road to v1 Apr 25, 2022
@adizere adizere linked a pull request Apr 25, 2022 that will close this issue
6 tasks
@adizere adizere moved this from Backlog to Needs Review in IBC-rs: the road to v1 Apr 28, 2022
@romac romac added the A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews label May 3, 2022
Repository owner moved this from Needs Review to Closed in IBC-rs: the road to v1 May 11, 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: bug Admin: something isn't working A: low-priority Admin: low priority / non urgent issue, expect longer wait time for PR reviews O: tests Objective: Test more aspect of the relayer
Projects
No open projects
Status: Closed
3 participants