-
Notifications
You must be signed in to change notification settings - Fork 639
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
transfer (total escrow): add invariant for total escrow #3506
Changes from 3 commits
fc6db2b
4361d2b
43a06e3
d3d3bda
87240a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
package keeper | ||
|
||
import ( | ||
"fmt" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
||
"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" | ||
) | ||
|
||
// RegisterInvariants registers all transfer invariants | ||
func RegisterInvariants(ir sdk.InvariantRegistry, k *Keeper) { | ||
ir.RegisterRoute(types.ModuleName, "total-escrow-per-denom", | ||
TotalEscrowPerDenomInvariants(k)) | ||
} | ||
|
||
// AllInvariants runs all invariants of the transfer module. | ||
func AllInvariants(k *Keeper) sdk.Invariant { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should add a call to this in the tests? |
||
return func(ctx sdk.Context) (string, bool) { | ||
return TotalEscrowPerDenomInvariants(k)(ctx) | ||
} | ||
} | ||
|
||
// TotalEscrowPerDenomInvariants checks that the total amount escrowed for | ||
// each denom is not smaller than the amount stored in the state entry. | ||
func TotalEscrowPerDenomInvariants(k *Keeper) sdk.Invariant { | ||
return func(ctx sdk.Context) (string, bool) { | ||
var actualTotalEscrowed sdk.Coins | ||
|
||
expectedTotalEscrowed := k.GetAllTotalEscrowed(ctx) | ||
|
||
portID := k.GetPort(ctx) | ||
transferChannels := k.channelKeeper.GetAllChannelsWithPortPrefix(ctx, portID) | ||
for _, channel := range transferChannels { | ||
escrowAddress := types.GetEscrowAddress(portID, channel.ChannelId) | ||
escrowBalances := k.bankKeeper.GetAllBalances(ctx, escrowAddress) | ||
|
||
actualTotalEscrowed = actualTotalEscrowed.Add(escrowBalances...) | ||
} | ||
|
||
// the actual escrowed amount must be greater than or equal to the expected amount for all denominations | ||
if !actualTotalEscrowed.IsAllGTE(expectedTotalEscrowed) { | ||
return sdk.FormatInvariant( | ||
types.ModuleName, | ||
"total escrow per denom invariance", | ||
fmt.Sprintf("found denom(s) with total escrow amount lower than expected:\nactual total escrowed: %s\nexpected total escrowed: %s", actualTotalEscrowed, expectedTotalEscrowed)), true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The full error msg isn't as nice, as it'll contain all denoms rather than just the ones of out sync, but I find this code a lot simpler and easier to verify correctness. This should be a very rare invariant to break (hopefully it never breaks) |
||
} | ||
|
||
return "", false | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
package keeper_test | ||
|
||
import ( | ||
"cosmossdk.io/math" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/cosmos/ibc-go/v7/modules/apps/transfer/keeper" | ||
"github.com/cosmos/ibc-go/v7/modules/apps/transfer/types" | ||
) | ||
|
||
func (suite *KeeperTestSuite) TestTotalEscrowPerDenomInvariant() { | ||
testCases := []struct { | ||
name string | ||
malleate func() | ||
expPass bool | ||
}{ | ||
{ | ||
"success", | ||
func() {}, | ||
true, | ||
}, | ||
{ | ||
"fails with broken invariant", | ||
func() { | ||
// set amount for denom higher than actual value in escrow | ||
amount := math.NewInt(200) | ||
suite.chainA.GetSimApp().TransferKeeper.SetTotalEscrowForDenom(suite.chainA.GetContext(), sdk.DefaultBondDenom, amount) | ||
}, | ||
false, | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
tc := tc | ||
|
||
suite.Run(tc.name, func() { | ||
suite.SetupTest() // reset | ||
path := NewTransferPath(suite.chainA, suite.chainB) | ||
suite.coordinator.Setup(path) | ||
|
||
amount := math.NewInt(100) | ||
|
||
// send coins from chain A to chain B so that we have them in escrow | ||
coin := sdk.NewCoin(sdk.DefaultBondDenom, amount) | ||
msg := types.NewMsgTransfer( | ||
path.EndpointA.ChannelConfig.PortID, | ||
path.EndpointA.ChannelID, | ||
coin, | ||
suite.chainA.SenderAccount.GetAddress().String(), | ||
suite.chainB.SenderAccount.GetAddress().String(), | ||
suite.chainA.GetTimeoutHeight(), 0, "", | ||
) | ||
|
||
res, err := suite.chainA.SendMsgs(msg) | ||
suite.Require().NoError(err) | ||
suite.Require().NotNil(res) | ||
|
||
tc.malleate() | ||
|
||
out, broken := keeper.TotalEscrowPerDenomInvariants(&suite.chainA.GetSimApp().TransferKeeper)(suite.chainA.GetContext()) | ||
|
||
if tc.expPass { | ||
suite.Require().False(broken) | ||
suite.Require().Empty(out) | ||
} else { | ||
suite.Require().True(broken) | ||
suite.Require().NotEmpty(out) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this string is arbitrary and doesn't hold too much significance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't seem to be treated as very significant in the sdk equivalent impl.