Minor IBC integration-test improvements & fixes

[ysv]

This change is 

[dzmitryhil]

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

---

integration-tests/chain.go line 212 at r1 (raw file):

		defer requestCancel()

		balanceRes, err := bankClient.Balance(requestCtx, &banktypes.QueryBalanceRequest{

I did it intentionally, to because you might use wrong denom during the development, and that error will show you the current state.

[dzmitryhil]

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

[dzmitryhil]

Change the PR to Draft probably? Is see the commented code in it, but I guess it's temporary.

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

[ysv]

yes my fault
fixed now

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

---

integration-tests/chain.go line 212 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I did it intentionally, to because you might use wrong denom during the development, and that error will show you the current state.

I see
Rollbacked & added comment there

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

	coreumRecipient := coreumChain.GenAccount()

	sendToCoreumCoin := gaiaChain.NewCoin(sdk.NewInt(2000))

IMO it is beneficial to send different amounts in different tests to make sure we don't have accidental successes because of using the same amounts. I had this confusion while running tests because I had failurs & in both TestIBCTransferFromCoreumToGaiaAndBack & TestIBCTransferFromGaiaToCoreumAndBack we have same amount 1000 & they run in parallel

Maybe it is even better to use rand.Int in such places

WDYT ?

---

integration-tests/ibc/helpers.go line 3 at r3 (raw file):

//go:build integrationtests

package ibc

since this code is only used inside integration-tests/ibc I think it is reasonable to store it here

[wojtek-coreum]

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

---

integration-tests/chain.go line 235 at r6 (raw file):

// GetIBCChannelID returns the first opened channel of the IBC connected chain peer.
func (c ChainContext) GetIBCChannelID(ctx context.Context, destChainID string) (string, error) {

Minor: I like "peer" word more and I think we use it consistently everywhere

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

IMO it is beneficial to send different amounts in different tests to make sure we don't have accidental successes because of using the same amounts. I had this confusion while running tests because I had failurs & in both TestIBCTransferFromCoreumToGaiaAndBack & TestIBCTransferFromGaiaToCoreumAndBack we have same amount 1000 & they run in parallel

Maybe it is even better to use rand.Int in such places

WDYT ?

Agree to use different amounts but let's avoid randomness

[miladz68]

Reviewed 3 of 4 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @ysv)

[ysv]

Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @wojtek-coreum)

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Agree to use different amounts but let's avoid randomness

agree, will keep hardcoded but different amounts

[dzmitryhil]

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

---

integration-tests/ibc/helpers.go line 1 at r7 (raw file):

//go:build integrationtests

Let's keep the file name, and still call it ibc.go.

[dzmitryhil]

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

[miladz68]

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

[dzmitryhil]

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

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

agree, will keep hardcoded but different amounts

But how does the different amount help?

[dzmitryhil]

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

[ysv]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But how does the different amount help?

because these amounts might unintentionally match and will not detect a mistake in tests

[ysv]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

because these amounts might unintentionally match and will not detect a mistake in tests

same issue as we have here:
https://github.com/CoreumFoundation/coreum/pull/503/files#diff-fdb4e65de50064f97071dbab024f3e4655b8c86de59115e67b47e8197824b068R23

Since all channels are named channel-0 we have mistake in our tests but because of same naming they pass

[dzmitryhil]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

same issue as we have here:
https://github.com/CoreumFoundation/coreum/pull/503/files#diff-fdb4e65de50064f97071dbab024f3e4655b8c86de59115e67b47e8197824b068R23

Since all channels are named channel-0 we have mistake in our tests but because of same naming they pass

But the accounts are new each time, if the issue with the amount the account won't receive it.

[ysv]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But the accounts are new each time, if the issue with the amount the account won't receive it.

What are the benefits of using same amounts ?

IMO there are no. While using different will protect agains these type of issues, it has already happened for channel names that is why it is always better to use different amounts as a rule, maybe in this case it is unlikely to happen but it helps in general. Also it makes debugging easier.

Debugging example:
E.g when I see 2000 in logs I know that refer to a specific test. But if I see 1000 everywhere it could any tests

Screenshot 2023-05-22 at 11.31.12

[dzmitryhil]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

What are the benefits of using same amounts ?

IMO there are no. While using different will protect agains these type of issues, it has already happened for channel names that is why it is always better to use different amounts as a rule, maybe in this case it is unlikely to happen but it helps in general. Also it makes debugging easier.

Debugging example:
E.g when I see 2000 in logs I know that refer to a specific test. But if I see 1000 everywhere it could any tests

Screenshot 2023-05-22 at 11.31.12

OK, let's leave that comment until we discuss the https://github.com/CoreumFoundation/coreum/pull/503/files#diff-fdb4e65de50064f97071dbab024f3e4655b8c86de59115e67b47e8197824b068R23

[ysv]

Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @dzmitryhil, @miladz68, @silverspase, and @wojtek-coreum)

---

integration-tests/ibc/transfer_test.go line 64 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

OK, let's leave that comment until we discuss the https://github.com/CoreumFoundation/coreum/pull/503/files#diff-fdb4e65de50064f97071dbab024f3e4655b8c86de59115e67b47e8197824b068R23

rollbacked

[dzmitryhil]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: 3 of 5 files reviewed, all discussions resolved (waiting on @miladz68, @silverspase, and @wojtek-coreum)

[dzmitryhil]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68, @silverspase, and @wojtek-coreum)

[miladz68]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase and @wojtek-coreum)