imp (api)!: convert coins to token only once in MsgTransfer

[bznein]

Description

closes: #7080

---

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.

• Targeted PR against the correct branch (see CONTRIBUTING.md).
• Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
• Code follows the module structure standards and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/).
• Added relevant godoc comments.
• Provide a conventional commit message to follow the repository standards.
• Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
• Re-reviewed Files changed in the GitHub PR explorer.
• Review SonarCloud Report in the comment section below once CI passes.

[bznein]

This is an API-breaking change but since I assume the whole feature branch will this should not be an issue

[crodriguezvega]

Please correct me if I am wrong, but since this function is in a _test.go file, then it is not part of the public API, right?

[bznein]

Yeah that's a good point, I wasn't too sure about it tbh!

[bznein]

This function can technically be moved out of the keeper now, since it does need to read state to convert coins to token anymore. Happy to do it if it seems the best thing to do

[colin-axner]

Makes sense to me!

[bznein]

I don't believe we need this test anymore, because this would have failed during the conversion we don't perform anymore

[colin-axner]

Nice! Great work. The changes look good to me. It's nice to be more efficient in this approach

[crodriguezvega]

Sorry for leaving the review after merge, but is there a reason why we couldn't merge this directly to main and then backport to v9? Then we also have these changes in the release branch. As far as I can see there is nothing in the PR that directly depends on the port router refactor, right?

[crodriguezvega]

Please correct me if I am wrong, but since this function is in a _test.go file, then it is not part of the public API, right?

[bznein]

Sorry for leaving the review after merge, but is there a reason why we couldn't merge this directly to main and then backport to v9? Then we also have these changes in the release branch. As far as I can see there is nothing in the PR that directly depends on the port router refactor, right?

You're right, I could have done it but just automatically went on "feature branch" mode :) I think it should be safe to add the same PR for main, I can do it

[bznein]

Sorry for leaving the review after merge, but is there a reason why we couldn't merge this directly to main and then backport to v9? Then we also have these changes in the release branch. As far as I can see there is nothing in the PR that directly depends on the port router refactor, right?

@crodriguezvega actually scratch my previous comment. On main we don't have the inefficient looping (

ibc-go/modules/apps/transfer/keeper/msg_server.go

Lines 30 to 51 in 9efb710

	coins := msg.GetCoins()
	if err := k.bankKeeper.IsSendEnabledCoins(ctx, coins...); err != nil {
	return nil, errorsmod.Wrapf(types.ErrSendDisabled, err.Error())
	}
	if k.isBlockedAddr(sender) {
	return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", sender)
	}
	if msg.Forwarding.GetUnwind() {
	msg, err = k.unwindHops(ctx, msg)
	if err != nil {
	return nil, err
	}
	}
	sequence, err := k.sendTransfer(
	ctx, msg.SourcePort, msg.SourceChannel, coins, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
	msg.Memo, msg.Forwarding.GetHops())
	if err != nil {
	return nil, err

) as we don't need to convert coins to tokens in the Transfer function since we use k.sendTransfer that takes coins).

(inside that function we still convert coins to token but we also use coins directly, so it's a bit more complex. I guess if we want this change in main we can make it but better to create another issue as it is definitely more involved.

[crodriguezvega]

Sorry for leaving the review after merge, but is there a reason why we couldn't merge this directly to main and then backport to v9? Then we also have these changes in the release branch. As far as I can see there is nothing in the PR that directly depends on the port router refactor, right?

@crodriguezvega actually scratch my previous comment. On main we don't have the inefficient looping (

ibc-go/modules/apps/transfer/keeper/msg_server.go

Lines 30 to 51 in 9efb710

	coins := msg.GetCoins()
	if err := k.bankKeeper.IsSendEnabledCoins(ctx, coins...); err != nil {
	return nil, errorsmod.Wrapf(types.ErrSendDisabled, err.Error())
	}
	if k.isBlockedAddr(sender) {
	return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "%s is not allowed to send funds", sender)
	}
	if msg.Forwarding.GetUnwind() {
	msg, err = k.unwindHops(ctx, msg)
	if err != nil {
	return nil, err
	}
	}
	sequence, err := k.sendTransfer(
	ctx, msg.SourcePort, msg.SourceChannel, coins, sender, msg.Receiver, msg.TimeoutHeight, msg.TimeoutTimestamp,
	msg.Memo, msg.Forwarding.GetHops())
	if err != nil {
	return nil, err

) as we don't need to convert coins to tokens in the Transfer function since we use k.sendTransfer that takes coins).
(inside that function we still convert coins to token but we also use coins directly, so it's a bit more complex. I guess if we want this change in main we can make it but better to create another issue as it is definitely more involved.

Ah, gotcha. Thank you for explaining!

[chatton]

thanks for picking this one up, LGTM