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.

Sign up for GitHub

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

Jump to bottom

[CORE-709] do not error when completing a bridge with non-positive amount #691

Merged
merged 3 commits into from
Oct 25, 2023
Merged

[CORE-709] do not error when completing a bridge with non-positive amount #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5eca091
do not error when completing a bridge with non-positive amount
tqin7 Oct 24, 2023
b708416
Merge branch 'main' into tq/core-709
tqin7 Oct 25, 2023
859030f
address nit
tqin7 Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions protocol/x/bridge/keeper/complete_bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,17 @@ func (k Keeper) CompleteBridge(
return err
}

// Send coin from bridge module account to specified account.
if err = k.bankKeeper.SendCoinsFromModuleToAccount(
ctx,
types.ModuleName,
bridgeAccAddress,
sdk.Coins{bridge.Coin},
); err != nil {
return err
// If coin amount is positive, send coin from bridge module account to
// specified account.
if bridge.Coin.Amount.IsPositive() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we have taken the approach to still schedule delayed messages for 0 transfers.
Can you document somewhere the rationale behind tradeoffs of different approaches, and why we are allowing zero-transfer delay messages to be added to state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented rationale in PR description!

if err = k.bankKeeper.SendCoinsFromModuleToAccount(
ctx,
types.ModuleName,
bridgeAccAddress,
sdk.Coins{bridge.Coin},
); err != nil {
return err
}
}

// Emit metric on last completed bridge id.
Expand Down
70 changes: 38 additions & 32 deletions protocol/x/bridge/keeper/complete_bridge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
func TestCompleteBridge(t *testing.T) {
tests := map[string]struct {
// Initial balance of bridge module account.
initialBalance sdk.Coin
initialModAccBalance sdk.Coin
// Bridge event to complete.
bridgeEvent types.BridgeEvent
// Whether bridging is disabled.
Expand All @@ -24,26 +24,25 @@ func TestCompleteBridge(t *testing.T) {
// Expected error, if any.
expectedError string
// Expected balance of bridge module account after bridge completion.
expectedBalance sdk.Coin
expectedModAccBalance sdk.Coin
}{
"Success": {
initialBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
bridgeEvent: constants.BridgeEvent_Id0_Height0, // bridges 888 tokens.
expectedBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(112)), // 1000 - 888.
initialModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
bridgeEvent: constants.BridgeEvent_Id0_Height0, // bridges 888 tokens.
expectedModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(112)), // 1000 - 888.
},
"Failure: coin amount is 0": {
initialBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
"Success: coin amount is 0": {
initialModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
bridgeEvent: types.BridgeEvent{
Id: 7,
Address: constants.BobAccAddress.String(),
Coin: sdk.NewCoin("adv4tnt", sdkmath.ZeroInt()),
EthBlockHeight: 3,
},
expectedError: "invalid coin",
expectedBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
expectedModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
},
"Failure: coin amount is negative": {
initialBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
"Success: coin amount is negative": {
initialModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
bridgeEvent: types.BridgeEvent{
Id: 7,
Address: constants.BobAccAddress.String(),
Expand All @@ -53,32 +52,31 @@ func TestCompleteBridge(t *testing.T) {
},
EthBlockHeight: 3,
},
expectedError: "invalid coin",
expectedBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
expectedModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
},
"Failure: invalid address string": {
initialBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
initialModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
bridgeEvent: types.BridgeEvent{
Id: 4,
Address: "not an address string",
Coin: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1)),
EthBlockHeight: 2,
},
expectedError: "decoding bech32 failed",
expectedBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
expectedError: "decoding bech32 failed",
expectedModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
},
"Failure: bridge module account has insufficient balance": {
initialBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(500)),
bridgeEvent: constants.BridgeEvent_Id0_Height0, // bridges 888 tokens.
expectedError: "insufficient funds",
expectedBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(500)),
initialModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(500)),
bridgeEvent: constants.BridgeEvent_Id0_Height0, // bridges 888 tokens.
expectedError: "insufficient funds",
expectedModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(500)),
},
"Failure: bridging is disabled": {
initialBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
bridgeEvent: constants.BridgeEvent_Id0_Height0, // bridges 888 tokens.
bridgingDisabled: true,
expectedError: types.ErrBridgingDisabled.Error(),
expectedBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)), // same as initial.
initialModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)),
bridgeEvent: constants.BridgeEvent_Id0_Height0, // bridges 888 tokens.
bridgingDisabled: true,
expectedError: types.ErrBridgingDisabled.Error(),
expectedModAccBalance: sdk.NewCoin("adv4tnt", sdkmath.NewInt(1_000)), // same as initial.
},
}

Expand All @@ -91,11 +89,11 @@ func TestCompleteBridge(t *testing.T) {
DelayBlocks: bridgeKeeper.GetSafetyParams(ctx).DelayBlocks,
})
require.NoError(t, err)
// Fund bridge module account with enought balance.
// Fund bridge module account with enough balance.
err = bankKeeper.MintCoins(
ctx,
types.ModuleName,
sdk.NewCoins(tc.initialBalance),
sdk.NewCoins(tc.initialModAccBalance),
)
require.NoError(t, err)

Expand All @@ -115,13 +113,21 @@ func TestCompleteBridge(t *testing.T) {
sdk.MustAccAddressFromBech32(tc.bridgeEvent.Address),
tc.bridgeEvent.Coin.Denom,
)
require.Equal(t, tc.bridgeEvent.Coin.Denom, balance.Denom)
require.Equal(t, tc.bridgeEvent.Coin.Amount, balance.Amount)
expectedBalance := sdk.NewCoin(tc.bridgeEvent.Coin.Denom, sdkmath.ZeroInt())
if tc.bridgeEvent.Coin.Amount.IsPositive() {
expectedBalance = tc.bridgeEvent.Coin
}
require.Equal(t, expectedBalance.Denom, balance.Denom)
require.Equal(t, expectedBalance.Amount, balance.Amount)
}
// Assert that bridge module account's balance is as expected.
modAccBalance := bankKeeper.GetBalance(ctx, types.ModuleAddress, tc.bridgeEvent.Coin.Denom)
require.Equal(t, tc.expectedBalance.Denom, modAccBalance.Denom)
require.Equal(t, tc.expectedBalance.Amount, modAccBalance.Amount)
modAccBalance := bankKeeper.GetBalance(
ctx,
types.ModuleAddress,
tc.bridgeEvent.Coin.Denom,
)
require.Equal(t, tc.expectedModAccBalance.Denom, modAccBalance.Denom)
require.Equal(t, tc.expectedModAccBalance.Amount, modAccBalance.Amount)
})
}
}
Loading