chore(api)!: move checks from Transfer to OnSendPacket #7068

bznein · 2024-08-07T13:01:03Z

Description

closes: #6949

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.

* deps: bump cosmos-sdk to v0.50.8 * chore: update changelog * deps: bump cosmossdk.io/client to v2.0.0-beta.3. bump x/upgrade to v0.1.4 * chore: make tidy-all * test: bump to 3f6796fba413cca for testing purposes. * deps: bump cosmos sdk to 0.50.9 * Update CHANGELOG.md * chore: update CHANGELOG for submodules. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>

…SendPacket

bznein · 2024-08-07T13:11:20Z

modules/apps/transfer/keeper/keeper.go

@@ -391,7 +391,7 @@ func (k Keeper) iterateForwardedPackets(ctx sdk.Context, cb func(packet types.Fo

 // IsBlockedAddr checks if the given address is allowed to send or receive tokens.
 // The module account is always allowed to send and receive tokens.
-func (k Keeper) isBlockedAddr(addr sdk.AccAddress) bool {
+func (k Keeper) IsBlockedAddr(addr sdk.AccAddress) bool {


This was needed in order to be able to call it from OnSendPacket

bznein · 2024-08-07T13:12:46Z

modules/apps/transfer/keeper/msg_server.go

@@ -33,27 +29,6 @@ func (k Keeper) Transfer(goCtx context.Context, msg *types.MsgTransfer) (*types.
 		return nil, errorsmod.Wrapf(types.ErrSendDisabled, err.Error())


This check was left in this function as the logic is not easy to implement on OnSendPacket, we would have to export BankKeeper but also add a lot of logic to convert coins back to tokens

This check is necessary to be moved. We can move it into keeper.OnSendPacket. There's already logic to convert tokens to coin there

Note: going forward, users can invoke OnSendPacket without using MsgTransfer, so it's unsafe to have any necessary checks remain in MsgTransfer

bznein · 2024-08-07T13:14:50Z

modules/apps/transfer/keeper/relay_test.go

@@ -169,21 +169,6 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
 		// 	},
 		// 	channeltypes.ErrInvalidPacket,
 		// },
-		{


This test now doesn't succeed due to the following logic:

Transfer does not check for the version/forwarding combination anymore

the function that creates the packet data will ignore everything in forwarding when the version is V1.
Note that there is still a check on OnSendPacket but it will not be triggered because Hops will not be present.

I'll open an issue to add tests specifically for OnSendPacket since we currently do not have them

It should? I wonder if this was not working because of the 1 -> 0 change?

ouch thanks, yeah it does. I'm trying to give up coffee starting today and you can see the results :(

modules/apps/transfer/ibc_module.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

colin-axner · 2024-08-07T13:52:37Z

modules/apps/transfer/keeper/relay.go

+			if k.IsBlockedAddr(sender) {
+				return errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", sender)
+			}
+


this is an unnecessary addition?

No idea how this got in, sorry :/

colin-axner · 2024-08-07T13:53:14Z

modules/apps/transfer/keeper/relay.go

@@ -430,7 +434,7 @@ func createPacketDataBytesFromVersion(appVersion, sender, receiver, memo string,
 	case types.V1:
 		// Sanity check, tokens must always be of length 1 if using app version V1.
 		if len(tokens) != 1 {
-			return nil, errorsmod.Wrapf(ibcerrors.ErrInvalidRequest, "cannot transfer multiple coins with %s", types.V1)
+			return nil, errorsmod.Wrapf(types.ErrInvalidVersion, "length of tokens must be equal to 1 if using %s version", types.V1)


would recommend doing changes like this against main since it is unrelated to the port router changes

this was the result of a weird merge, because we used to panic here so I added a similar change but not identical, will revert

colin-axner

Nice work! As a side note, I noticed #7080 which came from the initial refactor, but as it was not necessary for this work I opened a separate issue

bznein · 2024-08-07T15:29:12Z

Nice work! As a side note, I noticed #7080 which came from the initial refactor, but as it was not necessary for this work I opened a separate issue

Ah! We noticed the same with Cian and were about to open an issue :D thanks!

…pperToSendPacket

damiannolan · 2024-08-08T10:48:15Z

modules/apps/transfer/keeper/msg_server.go

there is some commented out code on L54 that can be removed now!

// packetData := types.NewFungibleTokenPacketData( // fullDenomPath, msg.Token.Amount.String(), sender.String(), msg.Receiver, msg.Memo, // )

…nsferWrapperToSendPacket" This reverts commit be9f558, reversing changes made to 36e4b05.

sonarcloud · 2024-08-08T10:59:41Z

Quality Gate passed for 'ibc-go'

Issues
0 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

damiannolan and others added 6 commits August 7, 2024 10:37

chore(api!): move checks from Transfer to OnSendPacket

7285344

Merge branch 'feat/port-router' into bznein/6949/msgTransferWrapperTo…

ca07c45

…SendPacket

Fix merge error

f0f21ff

fix to test

e0fc870

Add Require() back

e98e568

bznein commented Aug 7, 2024

View reviewed changes

bznein marked this pull request as ready for review August 7, 2024 13:15

bznein requested review from colin-axner, AdityaSripal, damiannolan, chatton and DimitrisJim as code owners August 7, 2024 13:15

bznein added priority PRs that need prompt reviews 05-port Issues concerns the 05-port submodule feat: port-router labels Aug 7, 2024

colin-axner reviewed Aug 7, 2024

View reviewed changes

modules/apps/transfer/ibc_module.go Outdated Show resolved Hide resolved

bznein and others added 2 commits August 7, 2024 14:47

Update modules/apps/transfer/ibc_module.go

3fec066

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

Linter

4d15a77

colin-axner reviewed Aug 7, 2024

View reviewed changes

bznein added 3 commits August 7, 2024 14:58

Removed wrong merge results

503e790

reintroduce test

ffa3343

Move check for isSendEnabledCoins to keeper

3dd7deb

colin-axner approved these changes Aug 7, 2024

View reviewed changes

bznein added 3 commits August 8, 2024 09:35

new tests

36e4b05

Merge remote-tracking branch 'origin' into bznein/6949/msgTransferWra…

be9f558

…pperToSendPacket

fixed test

77fbb3a

bznein requested a review from srdtrk as a code owner August 8, 2024 08:49

bznein requested review from crodriguezvega and gjermundgaraba as code owners August 8, 2024 08:49

bznein added 3 commits August 8, 2024 10:00

linter

ae8ba6b

Removed checks and added comment

a2d0b74

removed extra parentheses

4d87e21

damiannolan approved these changes Aug 8, 2024

View reviewed changes

bznein added 2 commits August 8, 2024 11:51

Revert "Merge remote-tracking branch 'origin' into bznein/6949/msgTra…

04de448

…nsferWrapperToSendPacket" This reverts commit be9f558, reversing changes made to 36e4b05.

Delete commented out code

87b2411

bznein merged commit 0c47e45 into feat/port-router Aug 8, 2024
62 of 65 checks passed

bznein deleted the bznein/6949/msgTransferWrapperToSendPacket branch August 8, 2024 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(api)!: move checks from Transfer to OnSendPacket #7068

chore(api)!: move checks from Transfer to OnSendPacket #7068

bznein commented Aug 7, 2024

bznein Aug 7, 2024

bznein Aug 7, 2024

colin-axner Aug 7, 2024

colin-axner Aug 7, 2024

bznein Aug 7, 2024

colin-axner Aug 7, 2024

bznein Aug 7, 2024

colin-axner Aug 7, 2024

bznein Aug 7, 2024

colin-axner Aug 7, 2024

bznein Aug 7, 2024

colin-axner left a comment

bznein commented Aug 7, 2024

damiannolan Aug 8, 2024

sonarcloud bot commented Aug 8, 2024

		@@ -33,27 +29,6 @@ func (k Keeper) Transfer(goCtx context.Context, msg types.MsgTransfer) (types.
		return nil, errorsmod.Wrapf(types.ErrSendDisabled, err.Error())

chore(api)!: move checks from Transfer to OnSendPacket #7068

chore(api)!: move checks from Transfer to OnSendPacket #7068

Conversation

bznein commented Aug 7, 2024

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

bznein commented Aug 7, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Aug 8, 2024

Quality Gate passed for 'ibc-go'