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

ibc-hooks: failing unit tests when using with ibc-go >= 7.1.0 #57

Closed
1 task done
xlab opened this issue Jul 11, 2023 · 4 comments
Closed
1 task done

ibc-hooks: failing unit tests when using with ibc-go >= 7.1.0 #57

xlab opened this issue Jul 11, 2023 · 4 comments

Comments

@xlab
Copy link
Contributor

xlab commented Jul 11, 2023

Hello! Recent changes in ibc-go 7.1.0 (see apps/transfer in release notes and specifically ADR-011 for the background) are breaking unit tests for ibc-hooks package. While it can still work fine with ibc-go 7.2.0, this mock of escrow in the test is not sufficient anymore:

// send funds to the escrow address to simulate a transfer from the ibc module
escrowAddress := transfertypes.GetEscrowAddress(recvPacket.GetDestPort(), recvPacket.GetDestChannel())
err := suite.App.BankKeeper.SendCoins(suite.Ctx, suite.TestAddress.GetAddress(), escrowAddress, sdk.NewCoins(sdk.NewInt64Coin("stake", 2)))

Will result in a panic like this:

receiver cosmos1axvk83fmkhz98wqzse6ds0aq53q99xs7cc0478yungfeflscex0qlg7stl unescrows 1stake
    suite.go:87: test panicked: negative coin amount: -1stake
        goroutine 12 [running]:
        runtime/debug.Stack()
        	/usr/local/go/src/runtime/debug/stack.go:24 +0x64

This is because escrow amounts now tracked in TransferKeeper as well, not only as balances in the bank module.

To properly simulate escrowing tokens without actually sending them, the caller must also set the total escrow amount in the TransferKeeper directly:

// SetTotalEscrowForDenom stores the total amount of source chain tokens that are in escrow.
// Amount is stored in state if and only if it is not equal to zero. The function will panic
// if the amount is negative.
func (k Keeper) SetTotalEscrowForDenom(ctx sdk.Context, coin sdk.Coin) 

I have made necessary changes in PR #56 please note that it supports versions prior to 7.1.0 as well, that don't have SetTotalEscrowForDenom available in TransferKeeper.

Tasks

@xlab xlab changed the title ibc-hooks: failing unit tests when using with ibc-go >= 0.7.1 ibc-hooks: failing unit tests when using with ibc-go >= 7.1.0 Jul 12, 2023
@akc2267
Copy link

akc2267 commented Aug 14, 2023

This needs to be assigned to someone from Osmosis

@jtieri
Copy link
Member

jtieri commented Sep 11, 2023

cc @nicolaslara

@nicolaslara
Copy link
Contributor

That makes a lot of sense. Thanks for the fix! (approved the PR)

@jtieri
Copy link
Member

jtieri commented Sep 25, 2023

Closed by #56

@jtieri jtieri closed this as completed Sep 25, 2023
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

No branches or pull requests

4 participants