-
Notifications
You must be signed in to change notification settings - Fork 202
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
feat(inflation): add burn method #1823
Changes from 1 commit
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 |
---|---|---|
|
@@ -89,3 +89,13 @@ func NewKeeper( | |
func (k Keeper) Logger(ctx sdk.Context) log.Logger { | ||
return ctx.Logger().With("module", "x/"+types.ModuleName) | ||
} | ||
|
||
func (k Keeper) Burn(ctx sdk.Context, coins sdk.Coins, sender sdk.AccAddress) error { | ||
if err := k.bankKeeper.SendCoinsFromAccountToModule( | ||
ctx, sender, types.ModuleName, coins, | ||
); err != nil { | ||
return err | ||
} | ||
|
||
return k.bankKeeper.BurnCoins(ctx, types.ModuleName, coins) | ||
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. Is there logging done in the bank keeper so this can be picked up by heart monitor? 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. We can use the query of the supply from the bank module
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. That is one thing missing from the tests. They don't check if the total supply response changes after burning |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
package keeper_test | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/NibiruChain/nibiru/x/common/testutil" | ||
"github.com/NibiruChain/nibiru/x/common/testutil/testapp" | ||
"github.com/NibiruChain/nibiru/x/inflation/types" | ||
) | ||
|
||
func init() { | ||
testapp.EnsureNibiruPrefix() | ||
} | ||
|
||
func TestBurn(t *testing.T) { | ||
testCases := []struct { | ||
name string | ||
sender sdk.AccAddress | ||
burnCoin sdk.Coin | ||
expectedErr error | ||
}{ | ||
{ | ||
name: "pass", | ||
sender: testutil.AccAddress(), | ||
burnCoin: sdk.NewCoin("nibiru", sdk.NewInt(100)), | ||
expectedErr: nil, | ||
}, | ||
} | ||
for _, tc := range testCases { | ||
t.Run(fmt.Sprintf("Case %s", tc.name), func(t *testing.T) { | ||
nibiruApp, ctx := testapp.NewNibiruTestAppAndContext() | ||
nibiruApp.BankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(tc.burnCoin)) | ||
nibiruApp.BankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, tc.sender, sdk.NewCoins(tc.burnCoin)) | ||
|
||
// Burn coins | ||
err := nibiruApp.InflationKeeper.Burn(ctx, sdk.NewCoins(tc.burnCoin), tc.sender) | ||
if tc.expectedErr != nil { | ||
require.EqualError(t, err, tc.expectedErr.Error()) | ||
} else { | ||
require.NoError(t, err) | ||
} | ||
}) | ||
} | ||
} | ||
k-yang marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,9 @@ type BankKeeper interface { | |
GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins | ||
SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error | ||
SendCoinsFromModuleToModule(ctx sdk.Context, senderModule, recipientModule string, amt sdk.Coins) error | ||
SendCoinsFromAccountToModule( | ||
ctx sdk.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins, | ||
) error | ||
Comment on lines
+24
to
+26
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 addition of Add documentation to the |
||
MintCoins(ctx sdk.Context, name string, amt sdk.Coins) error | ||
BurnCoins(ctx sdk.Context, name string, amt sdk.Coins) error | ||
HasSupply(ctx sdk.Context, denom string) bool | ||
|
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.
The
Burn
method in theKeeper
struct correctly transfers coins from an account to the module before burning them. This method should ensure that the coins are indeed transferred to a "burn" account or somehow marked as burned to prevent their reuse. Additionally, consider adding more detailed logging for auditability, especially for operations like burning coins which are critical to the monetary policy.Enhance the
Burn
method with detailed logging to improve traceability and auditability of burn operations.