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

send_tx methods should work with any transactions, including non-IBC transactions #2314

Closed
5 tasks
soareschen opened this issue Jun 17, 2022 · 3 comments
Closed
5 tasks
Milestone

Comments

@soareschen
Copy link
Contributor

Summary

The send_tx methods on ChainHandle should not fail when non-IBC transactions are submitted.

Problem Definition

Currently the send_tx methods determine whether a list of transactions are committed successfully to a chain is by doing the following:

  1. Poll for success response for all TX hashes.
  2. Turning the response TX hashes into IbcEvent.
  3. Return if the number of IbcEvents match the number of transactions.

The problem is that not every transaction is IBC-related. If a non-IBC message is submitted, there is no IbcEvent to exract from the TX response, even when the transaction is committed successfully. This result in the send_tx methods keep waiting for IbcEvents until the timeout has reach, and then returning error.

This has been an issue in #1979, in which the upgrade chain transaction is not an IBC message and does not produce any IbcEvent on succeed. As a result, the send_tx transaction failed even when the chain is upgraded successfully.

The same issue happens in #1981, in which there is a need to submit a MsgRegisterCounterpartyPayee transaction, which is a non-IBC message.

Generally, the same limitation probably also applies to all non-IBC messages, such as local token transfer, staking tokens, token swaps, calling smart contracts, etc. Although Hermes does not currently perform any of such transactions, it is an overly specialized limitation that should not be there.

It is also not practical to consider case-by-case which non-IBC message should be supported by send_tx methods. After all, the type signature of send_tx shows that it accepts Vec<Any>, and it does not make sense that the Any value is subtly restricted to an arbitrary subset of messages that are difficult to change.

Proposal

#2303 works on the fix to allow the transaction to succeed based only on TX response. The response is only converted after the fact into IbcEvents.

Acceptance Criteria

The send_tx methods should work with non-IBC messages.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ancazamfir
Copy link
Collaborator

The tx raw upgrade-chain is a helper test CLI we added in order to avoid the more complicated gaiad variant (which we were using initially in the upgrade testing)

What makes MsgRegisterCounterpartyPayee a non-IBC message? It is defined and handled in ibc-go repo, and while not an IBC core message is IBC middleware. Could you paste the events that result for a succesful Tx with a single MsgRegisterCounterpartyPayee message?

@ancazamfir
Copy link
Collaborator

ancazamfir commented Jun 20, 2022

The tx raw upgrade-chain is a helper test CLI we added in order to avoid the more complicated gaiad variant (which we were using initially in the upgrade testing)

following up on this, i just checked, UpgradeProposal is now defined as an IBC (core client) message. If it is an IBC message then we should have an IBC event defined. Opened an issue in ibc-go to get their input.
(more thoughts in #2285 (comment))

@romac romac changed the title send_tx methods should work with any transactions, including non-IBC transactions send_tx methods should work with any transactions, including non-IBC transactions Jun 27, 2022
@adizere
Copy link
Member

adizere commented Jul 26, 2022

Closing this as likely not necessary. For the moment, Hermes will continue to specialize on IBC-related transactions.

@adizere adizere closed this as completed Jul 26, 2022
@adizere adizere added this to the v1.0.0 milestone Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants