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

Fix E2E Memo test for broken versions #3028

Closed
1 of 3 tasks
chatton opened this issue Jan 20, 2023 · 2 comments · Fixed by #3096
Closed
1 of 3 tasks

Fix E2E Memo test for broken versions #3028

chatton opened this issue Jan 20, 2023 · 2 comments · Fixed by #3096
Assignees
Labels
Milestone

Comments

@chatton
Copy link
Contributor

chatton commented Jan 20, 2023

Summary of Bug

depends on #3026

The TestMsgTransfer_WithMemo is failing in certain configurations. See the following workflow to see the full failures.

The failing tests seem to all have this error:

RPC error -32603 - Internal error: tx (2F4F8F5D2FA006F9329153C4736CFEDC9F65924E6A5EC33E63065A310837D86D) not found

Expected Behaviour

The test should account for these versions and should pass.

Version

The following table shows which tests are broken.

Chain A Chain B Result
main v4.1.1 PASS
main v3.3.1 PASS
main v2.4.2 PASS
main v5.0.1 PASS
v4.1.1 main FAIL
v3.3.1 main FAIL
v2.4.2 main FAIL
v5.0.1 main FAIL

Steps to Reproduce

Run the TestMsgTransfer_WithMemo with any of these versions.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@chatton chatton added the e2e label Jan 20, 2023
@chatton chatton self-assigned this Jan 20, 2023
@chatton
Copy link
Contributor Author

chatton commented Jan 20, 2023

It looks like this is an issue with how the cosmos.BroadcastTx function works. We are discarding the tx response if the authtx query fails, which it will in the case of an unknown field. The test passing locally when updating the function in ibctest to

func BroadcastTx(ctx context.Context, broadcaster *Broadcaster, broadcastingUser
        }

        err = testutil.WaitForCondition(time.Second*30, time.Second*5, func() (bool, error) {
-               _, err := broadcaster.GetTxResponseBytes(ctx, broadcastingUser)
+               var err error
+               txBytes, err = broadcaster.GetTxResponseBytes(ctx, broadcastingUser)
                if err != nil {
                        return false, nil
                }
@@ -217,11 +218,6 @@ func BroadcastTx(ctx context.Context, broadcaster *Broadcaster, broadcastingUser
                return sdk.TxResponse{}, err
        }

-       txBytes, err = broadcaster.GetTxResponseBytes(ctx, broadcastingUser)
-       if err != nil {
-               return sdk.TxResponse{}, err
-       }
-
        respWithTxHash, err := broadcaster.UnmarshalTxResponseBytes(ctx, txBytes)
        if err != nil {
                return sdk.TxResponse{}, err
@@ -229,8 +225,13 @@ func BroadcastTx(ctx context.Context, broadcaster *Broadcaster, broadcastingUser

        resp, err := authTx.QueryTx(cc, respWithTxHash.TxHash)
        if err != nil {
-               return sdk.TxResponse{}, err
+               // if we fail to query the tx, it means an error occurred with the original message broadcast.
+               // we should return this instead.
+               originalResp, err := broadcaster.UnmarshalTxResponseBytes(ctx, txBytes)
+               if err != nil {
+                       return sdk.TxResponse{}, err
+               }
+               return originalResp, nil
        }
-
        return *resp, nil
 }

We can wait for the sdk 47 PR to get merged in ibc test before making these changes. I don't think this is a big priority anymore as this confirms that the test still works, it was just an issue with the broadcast function.

@crodriguezvega crodriguezvega moved this to Todo in ibc-go Jan 23, 2023
@crodriguezvega crodriguezvega moved this from Todo to In progress in ibc-go Jan 23, 2023
@crodriguezvega crodriguezvega added this to the v7.0.0 milestone Jan 23, 2023
@crodriguezvega crodriguezvega moved this from In progress to On hold in ibc-go Jan 23, 2023
@chatton
Copy link
Contributor Author

chatton commented Jan 31, 2023

I created a PR against the sdk 47 branch here

@chatton chatton moved this from On hold to In progress in ibc-go Jan 31, 2023
@chatton chatton moved this from In progress to In review in ibc-go Feb 1, 2023
@github-project-automation github-project-automation bot moved this from In review to Todo in ibc-go Feb 1, 2023
@damiannolan damiannolan moved this from Todo to Done in ibc-go Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done 🥳
Development

Successfully merging a pull request may close this issue.

2 participants