feat(perp): MsgWithdrawFromPerpFund sudo call as part of #1642 #1734
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,34 +23,42 @@ import ( | |
| // function is being used when it's called from the PerpKeeper.Admin struct. | ||
| type admin struct{ *Keeper } | ||
|
|
||
| // WithdrawFromPerpFund sends funds from the Perp Fund to the "to" address. | ||
| // | ||
| // Args: | ||
| // - ctx: Blockchain context holding the current state | ||
| // - amount: Amount of micro-NUSD to withdraw. | ||
| // - sender: Admin address registered in x/sudo | ||
| // - to: Recipient address | ||
| func (k admin) WithdrawFromPerpFund( | ||
| ctx sdk.Context, amount sdkmath.Int, sender, to sdk.AccAddress, denom string, | ||
| ) (err error) { | ||
| if err := k.SudoKeeper.CheckPermissions(sender, ctx); err != nil { | ||
|
There was a problem hiding this comment. I have some opinions related to checking permissions here. I created some PR here: #1751 There was a problem hiding this comment. This is a stateful validation, reading from the KV store of another module, so it's not really for the MsgServer There was a problem hiding this comment. Yeah. It does not change that principle if this call is still in the message server. MsgServer delegates validation of the request to the SudoKeeper (that he really is the one reading the KVStore, not the MsgServer), and if it is correct, it calls the Admin function. What I don't believe is in the admin function to validate the permissions. It is a thin line and a matter of convention, but I can show you some benefits of this slight separation. There was a problem hiding this comment. Checking a whitelist is not a message field validation. If the keeper method doesn't have the permission check, other modules that import the methods can sidestep the sudo, defeating the purpose for the check. It's not just a difference by convention. Switching There was a problem hiding this comment. The message field validation should be done by default out of the message server as long as you implement ValidateBasic(). Validating any field in the message server is duplicating computing cycles. What, then, is the use case of the MsgServer if we remove the ValidateBasic? There are two paradigms here: the MsgServer as orchestrator of keepers (my ideal) or the Keepers as holders of all the logic and being pure defensive (your ideal; this removes the need for MsgServer in my opinion as it becomes only a wrapper of the keeper). It is a matter of convention from the moment you agree on what it means to share some Keeper method. Is there any use case at this stage for CloseMarket to be public in the keeper? Try to avoid the what-if thing. I am open to anything as long as it makes sense, and we will follow the convention from now on. |
||
| return err | ||
| } | ||
|
|
||
| var collateralDenom string | ||
| if denom == "" { | ||
| denomFromState, err := k.Collateral.Get(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| collateralDenom = denomFromState | ||
| } else { | ||
| collateralDenom = denom | ||
| } | ||
|
|
||
| coinToSend := sdk.NewCoin(collateralDenom, amount) | ||
| if err = k.BankKeeper.SendCoinsFromModuleToAccount( | ||
| ctx, | ||
| /* from */ types.PerpFundModuleAccount, | ||
| /* to */ to, | ||
| /* amount */ sdk.NewCoins(coinToSend), | ||
| ); err != nil { | ||
| return err | ||
| } | ||
| ctx.EventManager().EmitEvent(sdk.NewEvent( | ||
| "withdraw_from_perp_fund", | ||
| sdk.NewAttribute("to", to.String()), | ||
| sdk.NewAttribute("funds", coinToSend.String()), | ||
| )) | ||
|
|
||
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.
A little bit of a more descriptive message like the one in the title would be nice. You will appreciate it when you search for this in the changelog in four years!