Code Cleanup: MsgLiquidStake (#651)

Closes: #XXX

## Context and purpose of the change
Cleaned up MsgLiquidStake

## Brief Changelog
* Cleaned up variable naming, commenting, logging, and errors
* Moved validation checks as upstream as possible so that they occur before any state changes (relevant to protocol directed liquid stakes)
* Added filter to GetDepositRecordsByEpochAndChain to only grab transfer records
* Added event emission after LS
* Added check to error if the LS would result in 0 stTokens


## Author's Checklist

I have...

- [ ] Run and PASSED locally all GAIA integration tests
- [ ] If the change is contentful, I either:
    - [ ] Added a new unit test OR 
    - [ ] Added test cases to existing unit tests
- [ ] OR this change is a trivial rework / code cleanup without any test coverage

If skipped any of the tests above, explain.


## Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] manually tested (if applicable)
- [ ] confirmed the author wrote unit tests for new logic
- [ ] reviewed documentation exists and is accurate


## Documentation and Release Note

  - [ ] Does this pull request introduce a new feature or user-facing behavior changes? 
  - [ ] Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`?
  - [ ] This pull request updates existing proto field values (and require a backend and frontend migration)? 
  - [ ] Does this pull request change existing proto field names (and require a frontend migration)?
  How is the feature or change documented? 
      - [ ] not applicable
      - [ ] jira ticket `XXX` 
      - [ ] specification (`x/<module>/spec/`) 
      - [ ] README.md 
      - [ ] not documented

dockernet/tests/integration_tests.bats

@@ -137,7 +137,6 @@ setup_file() {
   $STRIDE_MAIN_CMD tx stakeibc liquid-stake $STAKE_AMOUNT $HOST_DENOM --from $STRIDE_VAL -y 
 
   # sleep two block for the tx to settle on stride
-  WAIT_FOR_STRING $STRIDE_LOGS "\[MINT ST ASSET\] success on $HOST_CHAIN_ID"
   WAIT_FOR_BLOCK $STRIDE_LOGS 2
 
   # make sure IBC_DENOM went down

readme-docs/md/records_README.md

@@ -24,7 +24,7 @@ Deposit Records
 - `GetDepositRecord()`
 - `RemoveDepositRecord()`
 - `GetAllDepositRecord()`
-- `GetDepositRecordByEpochAndChain()`
+- `GetTransferDepositRecordByEpochAndChain()`
 
 Epoch Unbonding Records
 - `SetEpochUnbondingRecord()`

x/records/README.md

@@ -26,7 +26,7 @@ Deposit Records
 - `GetDepositRecord()`
 - `RemoveDepositRecord()`
 - `GetAllDepositRecord()`
-- `GetDepositRecordByEpochAndChain()`
+- `GetTransferDepositRecordByEpochAndChain()`
 
 Epoch Unbonding Records
 

x/records/keeper/deposit_record.go

@@ -101,10 +101,12 @@ func GetDepositRecordIDBytes(id uint64) []byte {
 	return bz
 }
 
-func (k Keeper) GetDepositRecordByEpochAndChain(ctx sdk.Context, epochNumber uint64, chainId string) (val *types.DepositRecord, found bool) {
+func (k Keeper) GetTransferDepositRecordByEpochAndChain(ctx sdk.Context, epochNumber uint64, chainId string) (val *types.DepositRecord, found bool) {
 	records := k.GetAllDepositRecord(ctx)
 	for _, depositRecord := range records {
-		if depositRecord.DepositEpochNumber == epochNumber && depositRecord.HostZoneId == chainId {
+		if depositRecord.DepositEpochNumber == epochNumber &&
+			depositRecord.HostZoneId == chainId &&
+			depositRecord.Status == types.DepositRecord_TRANSFER_QUEUE {
 			return &depositRecord, true
 		}
 	}

x/stakeibc/keeper/msg_server_liquid_stake.go

@@ -2,9 +2,7 @@ package keeper
 
 import (
 	"context"
-	"fmt"
 
-	sdkmath "cosmossdk.io/math"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	errorsmod "cosmossdk.io/errors"
@@ -14,123 +12,108 @@ import (
 	"github.com/Stride-Labs/stride/v6/x/stakeibc/types"
 )
 
+// Exchanges a user's native tokens for stTokens using the current redemption rate
+// The native tokens must live on Stride with an IBC denomination before this function is called
+// The typical flow consists, first, of a transfer of native tokens from the host zone to Stride,
+//    and then the invocation of this LiquidStake function
+//
+// WARNING: This function is invoked from the begin/end blocker in a way that does not revert partial state when
+//    an error is thrown (i.e. the execution is non-atomic).
+//    As a result, it is important that the validation steps are positioned at the top of the function,
+//    and logic that creates state changes (e.g. bank sends, mint) appear towards the end of the function
 func (k msgServer) LiquidStake(goCtx context.Context, msg *types.MsgLiquidStake) (*types.MsgLiquidStakeResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 
-	// Init variables
-	// deposit `amount` of `denom` token to the stakeibc module
-	// NOTE: Should we add an additional check here? This is a pretty important line of code
-	// NOTE: If sender doesn't have enough inCoin, this errors (error is hard to interpret)
-	// check that hostZone is registered
-	// strided tx stakeibc liquid-stake 100 uatom
+	// Get the host zone from the base denom in the message (e.g. uatom)
 	hostZone, err := k.GetHostZoneFromHostDenom(ctx, msg.HostDenom)
 	if err != nil {
-		k.Logger(ctx).Error(fmt.Sprintf("Host Zone not found for denom (%s)", msg.HostDenom))
-		return nil, errorsmod.Wrapf(types.ErrInvalidHostZone, "no host zone found for denom (%s)", msg.HostDenom)
+		return nil, errorsmod.Wrapf(types.ErrInvalidToken, "no host zone found for denom (%s)", msg.HostDenom)
 	}
 
+	// Error immediately if the host zone is halted
 	if hostZone.Halted {
-		k.Logger(ctx).Error(fmt.Sprintf("Host Zone halted for denom (%s)", msg.HostDenom))
 		return nil, errorsmod.Wrapf(types.ErrHaltedHostZone, "halted host zone found for denom (%s)", msg.HostDenom)
 	}
 
-	// get the sender address
-	sender, _ := sdk.AccAddressFromBech32(msg.Creator)
-	// get the coins to send, they need to be in the format {amount}{denom}
-	// is safe. The converse is not true.
-	ibcDenom := hostZone.GetIbcDenom()
-	coinString := msg.Amount.String() + ibcDenom
-	inCoin, err := sdk.ParseCoinNormalized(coinString)
+	// Get user and module account addresses
+	liquidStakerAddress, err := sdk.AccAddressFromBech32(msg.Creator)
 	if err != nil {
-		k.Logger(ctx).Error(fmt.Sprintf("failed to parse coin (%s)", coinString))
-		return nil, errorsmod.Wrapf(err, "failed to parse coin (%s)", coinString)
+		return nil, errorsmod.Wrapf(err, "user's address is invalid")
 	}
-
-	// Creator owns at least "amount" of inCoin
-	balance := k.bankKeeper.GetBalance(ctx, sender, ibcDenom)
-	if balance.IsLT(inCoin) {
-		k.Logger(ctx).Error(fmt.Sprintf("balance is lower than staking amount. staking amount: %v, balance: %v", msg.Amount, balance.Amount))
-		return nil, errorsmod.Wrapf(sdkerrors.ErrInsufficientFunds, "balance is lower than staking amount. staking amount: %v, balance: %v", msg.Amount, balance.Amount)
-	}
-	// check that the token is an IBC token
-	isIbcToken := types.IsIBCToken(ibcDenom)
-	if !isIbcToken {
-		k.Logger(ctx).Error("invalid token denom - denom is not an IBC token (%s)", ibcDenom)
-		return nil, errorsmod.Wrapf(types.ErrInvalidToken, "denom is not an IBC token (%s)", ibcDenom)
+	hostZoneAddress, err := sdk.AccAddressFromBech32(hostZone.Address)
+	if err != nil {
+		return nil, errorsmod.Wrapf(err, "host zone address is invalid")
 	}
 
-	// safety check: redemption rate must be above safety threshold
+	// Safety check: redemption rate must be within safety bounds
 	rateIsSafe, err := k.IsRedemptionRateWithinSafetyBounds(ctx, *hostZone)
 	if !rateIsSafe || (err != nil) {
-		errMsg := fmt.Sprintf("IsRedemptionRateWithinSafetyBounds check failed. hostZone: %s, err: %s", hostZone.String(), err.Error())
-		return nil, errorsmod.Wrapf(types.ErrRedemptionRateOutsideSafetyBounds, errMsg)
+		return nil, errorsmod.Wrapf(types.ErrRedemptionRateOutsideSafetyBounds, "HostZone: %s, err: %s", hostZone.ChainId, err.Error())
 	}
 
-	bech32ZoneAddress, err := sdk.AccAddressFromBech32(hostZone.Address)
-	if err != nil {
-		return nil, fmt.Errorf("could not bech32 decode address %s of zone with id: %s", hostZone.Address, hostZone.ChainId)
-	}
-	err = k.bankKeeper.SendCoins(ctx, sender, bech32ZoneAddress, sdk.NewCoins(inCoin))
-	if err != nil {
-		k.Logger(ctx).Error("failed to send tokens from Account to Module")
-		return nil, errorsmod.Wrap(err, "failed to send tokens from Account to Module")
-	}
-	// mint user `amount` of the corresponding stAsset
-	// NOTE: We should ensure that denoms are unique - we don't want anyone spoofing denoms
-	err = k.MintStAssetAndTransfer(ctx, sender, msg.Amount, msg.HostDenom)
-	if err != nil {
-		k.Logger(ctx).Error("failed to send tokens from Account to Module")
-		return nil, errorsmod.Wrapf(err, "failed to mint %s stAssets to user", msg.HostDenom)
-	}
-
-	// create a deposit record of these tokens (pending transfer)
+	// Grab the deposit record that will be used for record keeping
 	strideEpochTracker, found := k.GetEpochTracker(ctx, epochtypes.STRIDE_EPOCH)
 	if !found {
-		k.Logger(ctx).Error("failed to find stride epoch")
 		return nil, errorsmod.Wrapf(sdkerrors.ErrNotFound, "no epoch number for epoch (%s)", epochtypes.STRIDE_EPOCH)
 	}
-	// Does this use too much gas?
-	depositRecord, found := k.RecordsKeeper.GetDepositRecordByEpochAndChain(ctx, strideEpochTracker.EpochNumber, hostZone.ChainId)
+	depositRecord, found := k.RecordsKeeper.GetTransferDepositRecordByEpochAndChain(ctx, strideEpochTracker.EpochNumber, hostZone.ChainId)
 	if !found {
-		k.Logger(ctx).Error("failed to find deposit record")
-		return nil, errorsmod.Wrapf(sdkerrors.ErrNotFound, fmt.Sprintf("no deposit record for epoch (%d)", strideEpochTracker.EpochNumber))
+		return nil, errorsmod.Wrapf(sdkerrors.ErrNotFound, "no deposit record for epoch (%d)", strideEpochTracker.EpochNumber)
 	}
-	depositRecord.Amount = depositRecord.Amount.Add(msg.Amount)
-	k.RecordsKeeper.SetDepositRecord(ctx, *depositRecord)
 
-	k.hooks.AfterLiquidStake(ctx, sender)
-	return &types.MsgLiquidStakeResponse{}, nil
-}
-
-func (k msgServer) MintStAssetAndTransfer(ctx sdk.Context, sender sdk.AccAddress, amount sdkmath.Int, denom string) error {
-	stAssetDenom := types.StAssetDenomFromHostZoneDenom(denom)
+	// The tokens that are sent to the protocol are denominated in the ibc hash of the native token on stride (e.g. ibc/xxx)
+	nativeDenom := hostZone.IbcDenom
+	nativeCoin := sdk.NewCoin(nativeDenom, msg.Amount)
+	if !types.IsIBCToken(nativeDenom) {
+		return nil, errorsmod.Wrapf(types.ErrInvalidToken, "denom is not an IBC token (%s)", nativeDenom)
+	}
 
-	// TODO(TEST-7): Add an exchange rate here! What object should we store the exchange rate on?
-	// How can we ensure that the exchange rate is not manipulated?
-	hz, _ := k.GetHostZoneFromHostDenom(ctx, denom)
-	amountToMint := (sdk.NewDecFromInt(amount).Quo(hz.RedemptionRate)).TruncateInt()
-	coinString := amountToMint.String() + stAssetDenom
-	stCoins, err := sdk.ParseCoinsNormalized(coinString)
-	if err != nil {
-		k.Logger(ctx).Error("Failed to parse coins")
-		return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "Failed to parse coins %s", coinString)
+	// Confirm the user has a sufficient balance to execute the liquid stake
+	balance := k.bankKeeper.GetBalance(ctx, liquidStakerAddress, nativeDenom)
+	if balance.IsLT(nativeCoin) {
+		return nil, errorsmod.Wrapf(sdkerrors.ErrInsufficientFunds, "balance is lower than staking amount. staking amount: %v, balance: %v", msg.Amount, balance.Amount)
 	}
 
-	// Mints coins to the module account, will error if the module account does not exist or is unauthorized.
+	// Determine the amount of stTokens to mint using the redemption rate
+	stAmount := (sdk.NewDecFromInt(msg.Amount).Quo(hostZone.RedemptionRate)).TruncateInt()
+	if stAmount.IsZero() {
+		return nil, errorsmod.Wrapf(types.ErrInsufficientLiquidStake,
+			"Liquid stake of %s%s would return 0 stTokens", msg.Amount.String(), hostZone.HostDenom)
+	}
 
-	err = k.bankKeeper.MintCoins(ctx, types.ModuleName, stCoins)
-	if err != nil {
-		k.Logger(ctx).Error("Failed to mint coins")
-		return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "Failed to mint coins")
+	// Transfer the native tokens from the user to module account
+	if err := k.bankKeeper.SendCoins(ctx, liquidStakerAddress, hostZoneAddress, sdk.NewCoins(nativeCoin)); err != nil {
+		return nil, errorsmod.Wrap(err, "failed to send tokens from Account to Module")
 	}
 
-	// transfer those coins to the user
-	err = k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, sender, stCoins)
-	if err != nil {
-		k.Logger(ctx).Error("Failed to send coins from module to account")
-		return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "Failed to send %s from module to account", stCoins.GetDenomByIndex(0))
+	// Mint the stTokens and transfer them to the user
+	stDenom := types.StAssetDenomFromHostZoneDenom(msg.HostDenom)
+	stCoin := sdk.NewCoin(stDenom, stAmount)
+	if err := k.bankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(stCoin)); err != nil {
+		return nil, errorsmod.Wrapf(err, "Failed to mint coins")
+	}
+	if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, liquidStakerAddress, sdk.NewCoins(stCoin)); err != nil {
+		return nil, errorsmod.Wrapf(err, "Failed to send %s from module to account", stCoin.String())
 	}
 
-	k.Logger(ctx).Info(fmt.Sprintf("[MINT ST ASSET] success on %s.", hz.GetChainId()))
-	return nil
+	// Update the liquid staked amount on the deposit record
+	depositRecord.Amount = depositRecord.Amount.Add(msg.Amount)
+	k.RecordsKeeper.SetDepositRecord(ctx, *depositRecord)
+
+	// Emit liquid stake event
+	ctx.EventManager().EmitEvent(
+		sdk.NewEvent(
+			types.EventTypeLiquidStakeRequest,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
+			sdk.NewAttribute(types.AttributeKeyLiquidStaker, msg.Creator),
+			sdk.NewAttribute(types.AttributeKeyHostZone, hostZone.ChainId),
+			sdk.NewAttribute(types.AttributeKeyNativeBaseDenom, msg.HostDenom),
+			sdk.NewAttribute(types.AttributeKeyNativeIBCDenom, hostZone.IbcDenom),
+			sdk.NewAttribute(types.AttributeKeyNativeAmount, msg.Amount.String()),
+			sdk.NewAttribute(types.AttributeKeyStTokenAmount, stAmount.String()),
+		),
+	)
+
+	k.hooks.AfterLiquidStake(ctx, liquidStakerAddress)
+	return &types.MsgLiquidStakeResponse{}, nil
 }