(feat) Add possibility to transfer entire balance. #6877

bznein · 2024-07-18T13:57:37Z

Description

closes: #6567

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

crodriguezvega

Changes look good to me, @bznein. Maybe it would be worth adding a short e2e test and check that the balance of the account is zero after the transfer. Might be worth using 2 denoms in the e2e test.

bznein · 2024-07-19T11:01:51Z

Changes look good to me, @bznein. Maybe it would be worth adding a short e2e test and check that the balance of the account is zero after the transfer. Might be worth using 2 denoms in the e2e test.

Thanks Carlos! I've added the test, LMK if this is what you meant. FIY the test failed locally unless I manually built a new image and used that. I assume this would not happen in the CI.

DimitrisJim

this lgtm. thanks! 🙏 We should probably document this in the transfer docs too.

modules/apps/transfer/keeper/relay.go

Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>

gjermundgaraba

Nice add-on! LGTM 🚀

crodriguezvega

Thanks, @bznein. I left just one comment.

crodriguezvega · 2024-07-23T07:18:10Z

e2e/tests/transfer/base_test.go

+	})
+
+	t.Run("send entire balance from B to A", func(t *testing.T) {
+		transferTxResp := s.Transfer(ctx, chainB, chainBWallet, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, sdk.NewCoins(sdk.NewCoin(chainBIBCToken.IBCDenom(), transfertypes.UnboundedSpendLimit())), chainBAddress, chainAAddress, s.GetTimeoutHeight(ctx, chainB), 0, "", nil)


Should we transfer the whole balance for both the native chain B denom and the IBC denom? Just to test that it all works fine with multidenom.

Good idea, done

damiannolan

Nice work, lgtm!

testing/chain.go

modules/apps/transfer/types/token.go

damiannolan · 2024-07-23T08:01:19Z

e2e/tests/transfer/base_test.go

+	})
+
+	t.Run("send entire balance from B to A", func(t *testing.T) {
+		transferTxResp := s.Transfer(ctx, chainB, chainBWallet, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, sdk.NewCoins(sdk.NewCoin(chainBIBCToken.IBCDenom(), transfertypes.UnboundedSpendLimit())), chainBAddress, chainAAddress, s.GetTimeoutHeight(ctx, chainB), 0, "", nil)


CHANGELOG.md

crodriguezvega · 2024-07-23T08:05:51Z

@bznein Before merging the PR, should we address the latest comments?

bznein · 2024-07-23T08:11:37Z

@bznein Before merging the PR, should we address the latest comments?

Ah! Yeah sorry I added it at some point thiking it was ready to get merged (but it wasn't) and never removed it!

sonarcloud · 2024-07-23T08:18:30Z

Quality Gate passed for 'ibc-go'

Issues
17 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

* Add possibility to transfer entire balance. * Added entry to the Changelog. * Added e2e test * Added forwarding * Update modules/apps/transfer/keeper/relay.go Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * Move UnboundedSpendLimit to token.go * add documentation * add test to compatibility matrices * PR Feedback. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 92e1f38) # Conflicts: # .github/compatibility-test-matrices/release-v9.0.x/transfer-v2-multidenom-chain-a.json

* Add possibility to transfer entire balance. * Added entry to the Changelog. * Added e2e test * Added forwarding * Update modules/apps/transfer/keeper/relay.go Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * Move UnboundedSpendLimit to token.go * add documentation * add test to compatibility matrices * PR Feedback. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 92e1f38) # Conflicts: # .github/compatibility-test-matrices/main/transfer-v2-multidenom-chain-a.json # .github/compatibility-test-matrices/release-v9.0.x/transfer-v2-multidenom-chain-a.json # .github/compatibility-test-matrices/unreleased/transfer-v2-multidenom.json # CHANGELOG.md # docs/docs/02-apps/01-transfer/04-messages.md # docs/docs/02-apps/01-transfer/10-ICS20-v1/04-messages.md # e2e/tests/transfer/base_test.go # modules/apps/transfer/keeper/relay.go # modules/apps/transfer/keeper/relay_test.go # modules/apps/transfer/types/token.go # modules/apps/transfer/types/transfer_authorization.go # testing/chain.go

* Add possibility to transfer entire balance. * Added entry to the Changelog. * Added e2e test * Added forwarding * Update modules/apps/transfer/keeper/relay.go Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * Move UnboundedSpendLimit to token.go * add documentation * add test to compatibility matrices * PR Feedback. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 92e1f38) # Conflicts: # .github/compatibility-test-matrices/main/transfer-v2-multidenom-chain-a.json # .github/compatibility-test-matrices/release-v9.0.x/transfer-v2-multidenom-chain-a.json # .github/compatibility-test-matrices/unreleased/transfer-v2-multidenom.json # CHANGELOG.md # docs/docs/02-apps/01-transfer/04-messages.md # docs/docs/02-apps/01-transfer/10-ICS20-v1/04-messages.md # e2e/tests/transfer/base_test.go # modules/apps/transfer/keeper/relay.go # modules/apps/transfer/keeper/relay_test.go # modules/apps/transfer/types/token.go # modules/apps/transfer/types/transfer_authorization.go

…6929) * (feat) Add possibility to transfer entire balance. (#6877) * Add possibility to transfer entire balance. * Added entry to the Changelog. * Added e2e test * Added forwarding * Update modules/apps/transfer/keeper/relay.go Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * Move UnboundedSpendLimit to token.go * add documentation * add test to compatibility matrices * PR Feedback. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 92e1f38) # Conflicts: # .github/compatibility-test-matrices/main/transfer-v2-multidenom-chain-a.json # .github/compatibility-test-matrices/release-v9.0.x/transfer-v2-multidenom-chain-a.json # .github/compatibility-test-matrices/unreleased/transfer-v2-multidenom.json # CHANGELOG.md # docs/docs/02-apps/01-transfer/04-messages.md # docs/docs/02-apps/01-transfer/10-ICS20-v1/04-messages.md # e2e/tests/transfer/base_test.go # modules/apps/transfer/keeper/relay.go # modules/apps/transfer/keeper/relay_test.go # modules/apps/transfer/types/token.go # modules/apps/transfer/types/transfer_authorization.go # testing/chain.go * fix conflicts * lint * delete docs --------- Co-authored-by: Nikolas De Giorgis <nikolas.degiorgis@interchain.io> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

…6930) * (feat) Add possibility to transfer entire balance. (#6877) * Add possibility to transfer entire balance. * Added entry to the Changelog. * Added e2e test * Added forwarding * Update modules/apps/transfer/keeper/relay.go Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * Move UnboundedSpendLimit to token.go * add documentation * add test to compatibility matrices * PR Feedback. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 92e1f38) # Conflicts: # .github/compatibility-test-matrices/main/transfer-v2-multidenom-chain-a.json # .github/compatibility-test-matrices/release-v9.0.x/transfer-v2-multidenom-chain-a.json # .github/compatibility-test-matrices/unreleased/transfer-v2-multidenom.json # CHANGELOG.md # docs/docs/02-apps/01-transfer/04-messages.md # docs/docs/02-apps/01-transfer/10-ICS20-v1/04-messages.md # e2e/tests/transfer/base_test.go # modules/apps/transfer/keeper/relay.go # modules/apps/transfer/keeper/relay_test.go # modules/apps/transfer/types/token.go # modules/apps/transfer/types/transfer_authorization.go * fix conflicts * remove unused code --------- Co-authored-by: Nikolas De Giorgis <nikolas.degiorgis@interchain.io> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

…6920) * (feat) Add possibility to transfer entire balance. (#6877) * Add possibility to transfer entire balance. * Added entry to the Changelog. * Added e2e test * Added forwarding * Update modules/apps/transfer/keeper/relay.go Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> * Move UnboundedSpendLimit to token.go * add documentation * add test to compatibility matrices * PR Feedback. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 92e1f38) # Conflicts: # .github/compatibility-test-matrices/release-v9.0.x/transfer-v2-multidenom-chain-a.json * delete e2e * Revert "delete e2e" This reverts commit 71db49e. --------- Co-authored-by: Nikolas De Giorgis <nikolas.degiorgis@interchain.io> Co-authored-by: Carlos Rodriguez <carlos@interchain.io>

bznein added 2 commits July 17, 2024 10:44

Add possibility to transfer entire balance.

a61e4de

Added entry to the Changelog.

085281a

crodriguezvega added the backport-to-v9.0.x label Jul 18, 2024

crodriguezvega reviewed Jul 18, 2024

View reviewed changes

bznein added 2 commits July 19, 2024 11:18

Added e2e test

cb08050

Added forwarding

7fded12

bznein marked this pull request as ready for review July 19, 2024 11:01

bznein requested review from colin-axner, AdityaSripal, damiannolan, chatton, DimitrisJim and gjermundgaraba as code owners July 19, 2024 11:01

bznein requested a review from crodriguezvega July 19, 2024 11:01

DimitrisJim approved these changes Jul 19, 2024

View reviewed changes

modules/apps/transfer/keeper/relay.go Outdated Show resolved Hide resolved

DimitrisJim reviewed Jul 19, 2024

View reviewed changes

modules/apps/transfer/keeper/relay.go Show resolved Hide resolved

Update modules/apps/transfer/keeper/relay.go

70d4d8f

Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>

gjermundgaraba approved these changes Jul 19, 2024

View reviewed changes

bznein enabled auto-merge July 19, 2024 16:21

crodriguezvega added the priority PRs that need prompt reviews label Jul 22, 2024

Move UnboundedSpendLimit to token.go

b0fb227

crodriguezvega added backport-to-v7.7.x backport-to-v8.4.x labels Jul 23, 2024

crodriguezvega added 2 commits July 23, 2024 08:59

Merge branch 'main' into bznein/6567/TransferOfEntireBalance

c015920

add documentation

5b6f454

crodriguezvega requested a review from srdtrk as a code owner July 23, 2024 07:14

add test to compatibility matrices

ac19a1a

crodriguezvega approved these changes Jul 23, 2024

View reviewed changes

damiannolan approved these changes Jul 23, 2024

View reviewed changes

bznein added this pull request to the merge queue Jul 23, 2024

crodriguezvega removed this pull request from the merge queue due to a manual request Jul 23, 2024

PR Feedback.

ccfd659

bznein added this pull request to the merge queue Jul 23, 2024

Merged via the queue into main with commit 92e1f38 Jul 23, 2024
72 of 74 checks passed

bznein deleted the bznein/6567/TransferOfEntireBalance branch July 23, 2024 09:33

mergify bot mentioned this pull request Jul 23, 2024

(feat) Add possibility to transfer entire balance. (backport #6877) #6920

Merged

10 tasks

crodriguezvega added backport-to-v7.7.x backport-to-v8.4.x and removed backport-to-v7.7.x backport-to-v8.4.x labels Jul 23, 2024

This was referenced Jul 23, 2024

(feat) Add possibility to transfer entire balance. (backport #6877) #6929

Merged

(feat) Add possibility to transfer entire balance. (backport #6877) #6930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) Add possibility to transfer entire balance. #6877

(feat) Add possibility to transfer entire balance. #6877

bznein commented Jul 18, 2024 •

edited

Loading

crodriguezvega left a comment

bznein commented Jul 19, 2024

DimitrisJim left a comment

gjermundgaraba left a comment

crodriguezvega left a comment

crodriguezvega Jul 23, 2024

damiannolan Jul 23, 2024

bznein Jul 23, 2024

damiannolan left a comment

damiannolan Jul 23, 2024

crodriguezvega commented Jul 23, 2024

bznein commented Jul 23, 2024

sonarcloud bot commented Jul 23, 2024

(feat) Add possibility to transfer entire balance. #6877

(feat) Add possibility to transfer entire balance. #6877

Conversation

bznein commented Jul 18, 2024 • edited Loading

Description

crodriguezvega left a comment

Choose a reason for hiding this comment

bznein commented Jul 19, 2024

DimitrisJim left a comment

Choose a reason for hiding this comment

gjermundgaraba left a comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

crodriguezvega Jul 23, 2024

Choose a reason for hiding this comment

damiannolan Jul 23, 2024

Choose a reason for hiding this comment

bznein Jul 23, 2024

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Jul 23, 2024

Choose a reason for hiding this comment

crodriguezvega commented Jul 23, 2024

bznein commented Jul 23, 2024

sonarcloud bot commented Jul 23, 2024

Quality Gate passed for 'ibc-go'

bznein commented Jul 18, 2024 •

edited

Loading