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

improved TestIBCTransferFromGaiaToCoreumAndBack test #520

Merged
merged 7 commits into from
Jun 14, 2023

Conversation

vertex451
Copy link
Contributor

@vertex451 vertex451 commented Jun 4, 2023

This change is Reviewable

@vertex451 vertex451 requested a review from a team as a code owner June 4, 2023 12:52
@vertex451 vertex451 requested review from dzmitryhil, miladz68, ysv and wojtek-coreum and removed request for a team June 4, 2023 12:52
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)


integration-tests/ibc/transfer_test.go line 68 at r1 (raw file):

	// Fund accounts
	coreumChain.Faucet.FundAccounts(ctx, t, integrationtests.FundedAccount{

Messages: []sdk.Msg{&banksyptes.MsgSend{}}, is missing


integration-tests/ibc/transfer_test.go line 100 at r1 (raw file):

	)
	requireT.NoError(err)
	coreumChain.AwaitForBalance(ctx, t, coreumToGaiaSender, expectedBalanceAtCoreum)

you may check the final balance using normal bank client because the operation is done immediately

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)


integration-tests/ibc/transfer_test.go line 63 at r1 (raw file):

	// Generate accounts
	gaiaAccount := gaiaChain.GenAccount()

What is the issue with prev name gaiaSender ?


integration-tests/ibc/transfer_test.go line 68 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Messages: []sdk.Msg{&banksyptes.MsgSend{}}, is missing

Right and Amount: coreumChain.NewCoin(sdk.NewInt(1000000)), should be removed as well.


integration-tests/ibc/transfer_test.go line 80 at r1 (raw file):

	})

	// 1. Send from gaiaAccount to coreumToCoreumSender

Not sure that we need the 1. 2. indexes on the comment.

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

miladz68
miladz68 previously approved these changes Jun 8, 2023
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)

Copy link
Contributor Author

@vertex451 vertex451 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)


integration-tests/ibc/transfer_test.go line 63 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

What is the issue with prev name gaiaSender ?

Because it is simultaneously the sender and receiver, gaiaAccount is more precise for me.


integration-tests/ibc/transfer_test.go line 68 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Right and Amount: coreumChain.NewCoin(sdk.NewInt(1000000)), should be removed as well.

I switched coreumChain.Faucet.FundAccounts to coreumChain.FundAccountsWithOptions.


integration-tests/ibc/transfer_test.go line 80 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Not sure that we need the 1. 2. indexes on the comment.

Removed


integration-tests/ibc/transfer_test.go line 100 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

you may check the final balance using normal bank client because the operation is done immediately

I replaced that row with the following code:

	bankClient := banktypes.NewQueryClient(coreumChain.ClientContext)
	queryBalanceResponse, err := bankClient.Balance(ctx, &banktypes.QueryBalanceRequest{
		Address: coreumToGaiaSender.String(),
		Denom:   expectedBalanceAtCoreum.Denom,
	})
	requireT.NoError(err)
	assert.Equal(t, expectedBalanceAtCoreum.Amount, queryBalanceResponse.Balance.Amount)

Is it worth replacing? For me, the previous row seemed to be fine.

And also, I don't understand why a normal bank balance check guarantees the correct balance, what if we hit the moment before the send transaction will be included in a block?

miladz68
miladz68 previously approved these changes Jun 12, 2023
Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @wojtek-coreum, and @ysv)

Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wojtek-coreum and @ysv)

dzmitryhil
dzmitryhil previously approved these changes Jun 12, 2023
Copy link
Contributor

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @wojtek-coreum and @ysv)

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase and @ysv)


integration-tests/ibc/transfer_test.go line 100 at r1 (raw file):

Previously, silverspase (Artem) wrote…

I replaced that row with the following code:

	bankClient := banktypes.NewQueryClient(coreumChain.ClientContext)
	queryBalanceResponse, err := bankClient.Balance(ctx, &banktypes.QueryBalanceRequest{
		Address: coreumToGaiaSender.String(),
		Denom:   expectedBalanceAtCoreum.Denom,
	})
	requireT.NoError(err)
	assert.Equal(t, expectedBalanceAtCoreum.Amount, queryBalanceResponse.Balance.Amount)

Is it worth replacing? For me, the previous row seemed to be fine.

And also, I don't understand why a normal bank balance check guarantees the correct balance, what if we hit the moment before the send transaction will be included in a block?

But AwaitBalance have long timeout causing dealys if the test does not pass

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase and @ysv)


integration-tests/ibc/transfer_test.go line 108 at r2 (raw file):

	})
	requireT.NoError(err)
	assert.Equal(t, expectedBalanceAtCoreum.Amount, queryBalanceResponse.Balance.Amount)

Please use Amount.String() here to produce readable output

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @ysv)

@vertex451 vertex451 dismissed stale reviews from dzmitryhil and miladz68 via 4825d00 June 13, 2023 08:59
Copy link
Contributor Author

@vertex451 vertex451 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, @wojtek-coreum, and @ysv)


integration-tests/ibc/transfer_test.go line 100 at r1 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

But AwaitBalance have long timeout causing dealys if the test does not pass

Ah, I see


integration-tests/ibc/transfer_test.go line 108 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Please use Amount.String() here to produce readable output

Done

Copy link
Contributor

@miladz68 miladz68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wojtek-coreum and @ysv)

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dzmitryhil and @ysv)

@vertex451 vertex451 merged commit c86a439 into master Jun 14, 2023
@vertex451 vertex451 deleted the artem/improve-ibc-test branch June 14, 2023 13:15
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 this pull request may close these issues.

4 participants