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

feat(perp): MsgWithdrawFromPerpFund sudo call as part of #1642 #1734

Merged
merged 14 commits into from
Jan 1, 2024
Merged

feat(perp): MsgWithdrawFromPerpFund sudo call as part of #1642 #1734

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
240dffe
feat(perp): MsgDonateToPerpFund
Unique-Divine Dec 28, 2023
596a840
changelog
Unique-Divine Dec 28, 2023
5121341
Merge branch 'main' into realu/sg-perp
Unique-Divine Dec 28, 2023
0090d2f
linter
Unique-Divine Dec 28, 2023
e43672a
test assertiong
Unique-Divine Dec 28, 2023
d3f0ce3
test(perp-cli_test): Grab module acc addr programatically
Unique-Divine Dec 28, 2023
1316f18
Merge branch 'main' into realu/sg-perp
Unique-Divine Dec 28, 2023
cc89084
Merge branch 'main' into realu/sg-perp
Unique-Divine Dec 29, 2023
40aec00
Merge branch 'main' into realu/sg-perp
Unique-Divine Dec 29, 2023
b7fc955
Merge branch 'main' into realu/sg-perp
Unique-Divine Dec 30, 2023
1e2e226
consistent naming with #1752
Unique-Divine Dec 30, 2023
bf89c80
Merge branch 'main' into realu/sg-perp
k-yang Jan 1, 2024
a9938d4
Merge branch 'main' into realu/sg-perp
Unique-Divine Jan 1, 2024
11cb148
Merge branch 'main' into realu/sg-perp
Unique-Divine Jan 1, 2024
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
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* [#1573](https://github.com/NibiruChain/nibiru/pull/1573) - feat(perp): Close markets and compute settlement price
* [#1705](https://github.com/NibiruChain/nibiru/pull/1705) - feat(perp): Add oracle pair to market object
* [#1718](https://github.com/NibiruChain/nibiru/pull/1718) - fix: fees does not require additional funds
* [#1734](https://github.com/NibiruChain/nibiru/pull/1734) - feat(perp): MsgDonateToPerpFund sudo call as part of #1642
* [#1755](https://github.com/NibiruChain/nibiru/pull/1755) - feat(oracle): Add more events on validator's performance

### Non-breaking/Compatible Improvements
Expand All @@ -85,9 +86,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Dependencies

- Bump `github.com/prometheus/client_golang` from 1.17.0 to 1.18.0 ([#1750](https://github.com/NibiruChain/nibiru/pull/1750))
* Bump `github.com/prometheus/client_golang` from 1.17.0 to 1.18.0 ([#1750](https://github.com/NibiruChain/nibiru/pull/1750))
* Bump `google.golang.org/protobuf` from 1.31.0 to 1.32.0 ([#1756](https://github.com/NibiruChain/nibiru/pull/1756))

* Bump `google.golang.org/grpc` from 1.59.0 to 1.60.0 ([#1720](https://github.com/NibiruChain/nibiru/pull/1720))
* Bump `golang.org/x/crypto` from 0.15.0 to 0.17.0 ([#1724](https://github.com/NibiruChain/nibiru/pull/1724))
* Bump `github.com/holiman/uint256` from 1.2.3 to 1.2.4 ([#1730](https://github.com/NibiruChain/nibiru/pull/1730))
Expand Down
2 changes: 1 addition & 1 deletion app/keepers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ func ModuleAccPerms() map[string][]string {

perptypes.ModuleName: {},
perptypes.VaultModuleAccount: {},
perptypes.PerpEFModuleAccount: {},
perptypes.PerpFundModuleAccount: {},
perptypes.FeePoolModuleAccount: {},
perptypes.DNRAllocationModuleAccount: {},
perptypes.DNREscrowModuleAccount: {},
Expand Down
38 changes: 30 additions & 8 deletions proto/nibiru/perp/v2/tx.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ service Msg {
returns (MsgDonateToEcosystemFundResponse) {}

// ChangeCollateralDenom: Updates the collateral denom. A denom is valid if it
// is possible to make an sdk.Coin using it. [Admin] Only callable by sudoers.
// is possible to make an sdk.Coin using it. [SUDO] Only callable by sudoers.
rpc ChangeCollateralDenom(MsgChangeCollateralDenom)
returns (MsgChangeCollateralDenomResponse) {}

Expand All @@ -41,14 +41,19 @@ service Msg {
returns (MsgWithdrawEpochRebatesResponse) {}

// ShiftPegMultiplier: gRPC tx msg for changing a market's peg multiplier.
// [Admin] Only callable by sudoers.
// [SUDO] Only callable by sudoers.
rpc ShiftPegMultiplier(MsgShiftPegMultiplier)
returns (MsgShiftPegMultiplierResponse) {}

// ShiftSwapInvariant: gRPC tx msg for changing a market's swap invariant.
// [Admin] Only callable by sudoers.
// [SUDO] Only callable by sudoers.
rpc ShiftSwapInvariant(MsgShiftSwapInvariant)
returns (MsgShiftSwapInvariantResponse) {}

// WithdrawFromPerpFund: gRPC tx msg to withdraw from the perp fund module
// account. [SUDO] Only callable by sudoers.
rpc WithdrawFromPerpFund(MsgWithdrawFromPerpFund)
returns (MsgWithdrawFromPerpFundResponse) {}
}

// -------------------------- Settle Position --------------------------
Expand Down Expand Up @@ -353,7 +358,7 @@ message MsgDonateToEcosystemFundResponse {}
// ----------------------- MsgChangeCollateralDenom -----------------------

// MsgChangeCollateralDenom: Changes the collateral denom for the module.
// [Admin] Only callable by sudoers.
// [SUDO] Only callable by sudoers.
message MsgChangeCollateralDenom {
string sender = 1;
string new_denom = 2;
Expand Down Expand Up @@ -394,8 +399,8 @@ message MsgWithdrawEpochRebatesResponse {

// -------------------------- ShiftPegMultiplier --------------------------

// ShiftPegMultiplier: gRPC tx msg for changing the peg multiplier.
// Admin-only.
// MsgShiftPegMultiplier: gRPC tx msg for changing the peg multiplier.
// [SUDO] Only callable sudoers.
message MsgShiftPegMultiplier {
string sender = 1;
string pair = 2 [
Expand All @@ -413,8 +418,8 @@ message MsgShiftPegMultiplierResponse {}

// -------------------------- ShiftSwapInvariant --------------------------

// ShiftSwapInvariant: gRPC tx msg for changing the swap invariant.
// Admin-only.
// MsgShiftSwapInvariant: gRPC tx msg for changing the swap invariant.
// [SUDO] Only callable sudoers.
message MsgShiftSwapInvariant {
string sender = 1;
string pair = 2 [
Expand All @@ -429,3 +434,20 @@ message MsgShiftSwapInvariant {
}

message MsgShiftSwapInvariantResponse {}

// -------------------------- WithdrawFromPerpFund --------------------------

// MsgWithdrawFromPerpFund: gRPC tx msg for changing the swap invariant.
// [SUDO] Only callable sudoers.
message MsgWithdrawFromPerpFund {
string sender = 1;
string amount = 2 [
k-yang marked this conversation as resolved.
Show resolved Hide resolved
(gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int",
(gogoproto.nullable) = false
];
// Optional denom in case withdrawing assets aside from NUSD.
string denom = 3;
string to_addr = 4;
Copy link
Member

Choose a reason for hiding this comment

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

(nit) it might be safer to just withdraw to whoever sends the message, i.e. the message sender.

I can foresee possible human error in the future where somebody sends the message but accidentally sets the to_addr different from the message signer/sender.

Copy link
Member Author

@Unique-Divine Unique-Divine Dec 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think so too. The problem we might encounter here is when the claim goes to a smart contract that doesn't expose a function to withdraw its balance, or if there's routing happening between multiple contracts like the ones used in Mars Protocol or in future Nibiru-developed contracts.

sudo_contract_calls_msg -> withdraw_to_contract_addr -> No fn to withdraw -> funds are lost

I'm trying to avoid this ^ deadlock.

Adding an explicit to_addr makes it less error prone in a different way, by making the caller pass in the location they want the funds to end up.

Another option is to make that field nullable, so that empty means, to_addr := sender. Would you prefer that?

Copy link
Member

@k-yang k-yang Jan 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Wow, I didn't even consider that scenario. In that case, then I think it's better to be explicit.

Thinking about it more, in the general case, we'll likely be sending these WithdrawFromPerpFund messages from the CW3 multisig. Having the ability to target a recipient with the message would be more efficient, instead of withdrawing to the CW3 multisig and then doing a bank transfer.

A more sophisticated design could be to only execute the message if the to_addr is in the sudo sudoers list. It would prevent the funds from going to a CosmWasm smart contract that doesn't have the withdraw functionality, because we would be the ones whitelisting the possible recipients. But that could be addressed in a separate PR. What's implemented looks good to me.

}

message MsgWithdrawFromPerpFundResponse {}
16 changes: 2 additions & 14 deletions wasmbinding/bindings/msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,22 +14,15 @@ import (
type NibiruMsg struct {
// bindings-perp ExecuteMsg enum types
// MultiLiquidate *MultiLiquidate `json:"multi_liquidate,omitempty"` // TODO
DonateToInsuranceFund *DonateToInsuranceFund `json:"donate_to_insurance_fund,omitempty"` // TODO
InsuranceFundWithdraw *InsuranceFundWithdraw `json:"insurance_fund_withdraw,omitempty"`
SetMarketEnabled *SetMarketEnabled `json:"set_market_enabled,omitempty"`
CreateMarket *CreateMarket `json:"create_market,omitempty"`
SetMarketEnabled *SetMarketEnabled `json:"set_market_enabled,omitempty"`
CreateMarket *CreateMarket `json:"create_market,omitempty"`

EditOracleParams *EditOracleParams `json:"edit_oracle_params,omitempty"`

// Short for "no operation". A wasm binding payload that does nothing.
NoOp *NoOp `json:"no_op,omitempty"`
}

type DonateToInsuranceFund struct {
Sender string `json:"sender"`
Donation sdk.Coin `json:"donation"`
}

type EditOracleParams struct {
VotePeriod *sdkmath.Int `json:"vote_period,omitempty"`
VoteThreshold *sdk.Dec `json:"vote_threshold,omitempty"`
Expand All @@ -43,11 +36,6 @@ type EditOracleParams struct {
ValidatorFeeRatio *sdk.Dec `json:"validator_fee_ratio,omitempty"`
}

type InsuranceFundWithdraw struct {
Amount sdkmath.Int `json:"amount"`
To string `json:"to"`
}

type SetMarketEnabled struct {
Pair string `json:"pair"`
Enabled bool `json:"enabled"`
Expand Down
23 changes: 2 additions & 21 deletions wasmbinding/exec_perp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,6 @@ func (exec *ExecutorPerp) MsgServer() perpv2types.MsgServer {
return perpv2keeper.NewMsgServerImpl(exec.PerpV2)
}

func (exec *ExecutorPerp) InsuranceFundWithdraw(
cwMsg *bindings.InsuranceFundWithdraw, ctx sdk.Context,
) (err error) {
if cwMsg == nil {
return wasmvmtypes.InvalidRequest{Err: "null msg"}
}

to, err := sdk.AccAddressFromBech32(cwMsg.To)
if err != nil {
return err
}

return exec.PerpV2.Admin.WithdrawFromInsuranceFund(
ctx,
cwMsg.Amount,
to,
)
}

// TODO: rename to CloseMarket
func (exec *ExecutorPerp) SetMarketEnabled(
cwMsg *bindings.SetMarketEnabled, ctx sdk.Context,
Expand All @@ -52,7 +33,7 @@ func (exec *ExecutorPerp) SetMarketEnabled(
return err
}

return exec.PerpV2.Admin.CloseMarket(ctx, pair)
return exec.PerpV2.Sudo().CloseMarket(ctx, pair)
}

func (exec *ExecutorPerp) CreateMarket(
Expand Down Expand Up @@ -90,7 +71,7 @@ func (exec *ExecutorPerp) CreateMarket(
}
}

return exec.PerpV2.Admin.CreateMarket(ctx, perpv2keeper.ArgsCreateMarket{
return exec.PerpV2.Sudo().CreateMarket(ctx, perpv2keeper.ArgsCreateMarket{
Pair: pair,
PriceMultiplier: cwMsg.PegMult,
SqrtDepth: cwMsg.SqrtDepth,
Expand Down
32 changes: 0 additions & 32 deletions wasmbinding/exec_perp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,33 +134,13 @@ func (s *TestSuitePerpExecutor) OnSetupEnd() {
// Happy path coverage
func (s *TestSuitePerpExecutor) TestPerpExecutorHappy() {
for _, err := range []error{
s.DoInsuranceFundWithdrawTest(sdk.NewInt(69), s.contractDeployer),
s.DoCreateMarketTest(asset.MustNewPair("ufoo:ubar")),
s.DoCreateMarketTestWithParams(asset.MustNewPair("ufoo2:ubar")),
} {
s.NoError(err)
}
}

func (s *TestSuitePerpExecutor) DoInsuranceFundWithdrawTest(
amt sdkmath.Int, to sdk.AccAddress,
) error {
cwMsg := &bindings.InsuranceFundWithdraw{
Amount: amt,
To: to.String(),
}

err := testapp.FundModuleAccount(
s.nibiru.BankKeeper,
s.ctx,
perpv2types.PerpEFModuleAccount,
sdk.NewCoins(sdk.NewCoin(perpv2types.TestingCollateralDenomNUSD, sdk.NewInt(420))),
)
s.NoError(err)

return s.exec.InsuranceFundWithdraw(cwMsg, s.ctx)
}

func (s *TestSuitePerpExecutor) DoCreateMarketTest(pair asset.Pair) error {
cwMsg := &bindings.CreateMarket{
Pair: pair.String(),
Expand Down Expand Up @@ -197,11 +177,6 @@ func (s *TestSuitePerpExecutor) DoCreateMarketTestWithParams(pair asset.Pair) er
return s.exec.CreateMarket(cwMsg, s.ctx)
}

func (s *TestSuitePerpExecutor) TestSadPaths_Nil() {
err := s.exec.InsuranceFundWithdraw(nil, s.ctx)
s.Error(err)
}

func (s *TestSuitePerpExecutor) DoSetMarketEnabledTest(
pair asset.Pair, enabled bool,
) error {
Expand All @@ -220,13 +195,6 @@ func (s *TestSuitePerpExecutor) DoSetMarketEnabledTest(
return err
}

func (s *TestSuitePerpExecutor) TestSadPath_InsuranceFundWithdraw() {
fundsToWithdraw := sdk.NewCoin(perpv2types.TestingCollateralDenomNUSD, sdk.NewInt(69_000))

err := s.DoInsuranceFundWithdrawTest(fundsToWithdraw.Amount, s.contractDeployer)
s.Error(err)
}

func (s *TestSuitePerpExecutor) TestSadPaths_InvalidPair() {
sadPair := asset.Pair("ftt:ust:doge")
pair := sadPair
Expand Down
49 changes: 0 additions & 49 deletions wasmbinding/exec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,55 +313,6 @@ func (s *TestSuiteExecutor) TestNoOp() {
s.NoErrorf(err, "contractRespBz: %s", contractRespBz)
}

func (s *TestSuiteExecutor) TestInsuranceFundWithdraw() {
admin := s.contractDeployer.String()
amtToWithdraw := sdk.NewInt(69)
execMsg := bindings.NibiruMsg{
InsuranceFundWithdraw: &bindings.InsuranceFundWithdraw{
Amount: amtToWithdraw,
To: admin,
},
}

s.T().Log("Executing should fail since the IF doesn't have funds")
contract := s.contractController
s.keeper.SetSudoContracts(
[]string{contract.String()}, s.ctx,
)
contractRespBz, err := s.ExecuteAgainstContract(contract, execMsg)
s.Errorf(err, "contractRespBz: %s", contractRespBz)

s.T().Log("Executing without permission should fail")
s.keeper.SetSudoContracts(
[]string{}, s.ctx,
)
contractRespBz, err = s.ExecuteAgainstContract(contract, execMsg)
s.Errorf(err, "contractRespBz: %s", contractRespBz)

s.T().Log("Executing should work when the IF has funds")
err = testapp.FundModuleAccount(
s.nibiru.BankKeeper,
s.ctx,
perpv2types.PerpEFModuleAccount,
sdk.NewCoins(sdk.NewCoin(perpv2types.TestingCollateralDenomNUSD, sdk.NewInt(420))),
)
s.NoError(err)
s.keeper.SetSudoContracts(
[]string{contract.String()}, s.ctx,
)
contractRespBz, err = s.ExecuteAgainstContract(contract, execMsg)
s.NoErrorf(err, "contractRespBz: %s", contractRespBz)

s.T().Log("Executing the wrong contract should fail")
contract = s.contractPerp
s.keeper.SetSudoContracts(
[]string{contract.String()}, s.ctx,
)
contractRespBz, err = s.ExecuteAgainstContract(contract, execMsg)
s.Errorf(err, "contractRespBz: %s", contractRespBz)
s.Contains(err.Error(), "Error parsing into type")
}

func (s *TestSuiteExecutor) TestSetMarketEnabled() {
// admin := s.contractDeployer.String()
perpv2Genesis := genesis.PerpV2Genesis()
Expand Down
7 changes: 0 additions & 7 deletions wasmbinding/message_plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,6 @@ func (messenger *CustomMessenger) DispatchMsg(
err = messenger.Perp.CreateMarket(cwMsg, ctx)
return events, data, err

case contractExecuteMsg.ExecuteMsg.InsuranceFundWithdraw != nil:
if err := messenger.Sudo.CheckPermissions(contractAddr, ctx); err != nil {
return events, data, err
}
cwMsg := contractExecuteMsg.ExecuteMsg.InsuranceFundWithdraw
err = messenger.Perp.InsuranceFundWithdraw(cwMsg, ctx)
return events, data, err
case contractExecuteMsg.ExecuteMsg.SetMarketEnabled != nil:
if err := messenger.Sudo.CheckPermissions(contractAddr, ctx); err != nil {
return events, data, err
Expand Down
2 changes: 1 addition & 1 deletion x/oracle/keeper/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (h Hooks) AfterEpochEnd(ctx sdk.Context, epochIdentifier string, _ uint64)
}

if !totalRemainder.IsZero() {
err = h.bankKeeper.SendCoinsFromModuleToModule(ctx, perptypes.FeePoolModuleAccount, perptypes.PerpEFModuleAccount, totalRemainder)
err = h.bankKeeper.SendCoinsFromModuleToModule(ctx, perptypes.FeePoolModuleAccount, perptypes.PerpFundModuleAccount, totalRemainder)
if err != nil {
h.k.Logger(ctx).Error("Failed to send coins to perp ef module", "err", err)
}
Expand Down
2 changes: 1 addition & 1 deletion x/oracle/keeper/hooks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func TestHooks_AfterEpochEnd(t *testing.T) {
balances := app.BankKeeper.GetAllBalances(ctx, account.GetAddress())
assert.Equal(t, tt.expectedOracleBalances, balances)

account = app.AccountKeeper.GetModuleAccount(ctx, perptypes.PerpEFModuleAccount)
account = app.AccountKeeper.GetModuleAccount(ctx, perptypes.PerpFundModuleAccount)
balances = app.BankKeeper.GetAllBalances(ctx, account.GetAddress())
assert.Equal(t, tt.expectedEFBalances, balances)
})
Expand Down
12 changes: 9 additions & 3 deletions x/perp/v2/client/cli/cli_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

abcitypes "github.com/cometbft/cometbft/abci/types"
sdk "github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
bankcli "github.com/cosmos/cosmos-sdk/x/bank/client/cli"
"github.com/stretchr/testify/suite"

Expand Down Expand Up @@ -714,16 +715,21 @@ func (s *IntegrationTestSuite) TestDonateToEcosystemFund() {

s.NoError(s.network.WaitForNextBlock())
resp := new(sdk.Coin)
moduleAccountAddrPerpEF := "nibi1trh2mamq64u4g042zfeevvjk4cukrthvppfnc7"

moduleAccPerpFund := authtypes.NewModuleAddress(
types.PerpFundModuleAccount).String()
s.NoError(
testutilcli.ExecQuery(
s.network.Validators[0].ClientCtx,
bankcli.GetBalancesCmd(),
[]string{moduleAccountAddrPerpEF, "--denom", types.TestingCollateralDenomNUSD},
[]string{moduleAccPerpFund, "--denom", types.TestingCollateralDenomNUSD},
resp,
),
)
s.Require().EqualValues(sdk.NewInt64Coin(types.TestingCollateralDenomNUSD, 100), *resp)
s.Require().EqualValues(
sdk.NewInt64Coin(types.TestingCollateralDenomNUSD, 100).String(),
resp.String(),
)
}

func (s *IntegrationTestSuite) TestQueryModuleAccount() {
Expand Down
Loading
Loading