refactor(delegation): respond to ai comments

Instead of ignoring the error (even though we know it to be nil), examine the error and skip the loop if it is not nil. This is preferred over changing the function interface since it is more future proof.
ExocoreNetwork · Sep 23, 2024 · e418cba · e418cba
1 parent fedb3f7
commit e418cba
Show file tree

Hide file tree

Showing 4 changed files with 185 additions and 124 deletions.
diff --git a/x/delegation/keeper/abci.go b/x/delegation/keeper/abci.go
@@ -1,8 +1,6 @@
 package keeper
 
 import (
-	"strings"
-
 	assetstypes "github.com/ExocoreNetwork/exocore/x/assets/types"
 	"github.com/ExocoreNetwork/exocore/x/delegation/types"
 
@@ -13,92 +11,133 @@ import (
 
 // EndBlock : completed Undelegation events according to the canCompleted blockHeight
 // This function will be triggered at the end of every block, it will query the undelegation state to get the records that need to be handled and try to complete the undelegation task.
-func (k *Keeper) EndBlock(oCtx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate {
-	// #nosec G703 // the error is always nil
-	records, _ := k.GetPendingUndelegationRecords(oCtx, uint64(oCtx.BlockHeight()))
+func (k *Keeper) EndBlock(
+	originalCtx sdk.Context, _ abci.RequestEndBlock,
+) []abci.ValidatorUpdate {
+	logger := k.Logger(originalCtx)
+	records, err := k.GetPendingUndelegationRecords(
+		originalCtx, uint64(originalCtx.BlockHeight()),
+	)
+	if err != nil {
+		logger.Error("failed to get pending undelegation records", "error", err)
+	}
 	if len(records) == 0 {
+		logger.Info("no pending undelegation records")
 		return []abci.ValidatorUpdate{}
 	}
-	for _, record := range records {
-		ctx, writeCache := oCtx.CacheContext()
-		// check if the operator has been slashed or frozen
+	for i := range records {
+		record := records[i] // avoid implicit memory aliasing
+		ctx, writeCache := originalCtx.CacheContext()
+		// we can use `Must` here because we stored this record ourselves.
 		operatorAccAddress := sdk.MustAccAddressFromBech32(record.OperatorAddr)
-		// todo: don't think about freezing the operator in current implementation
-		/*		if k.slashKeeper.IsOperatorFrozen(ctx, operatorAccAddress) {
-				// reSet the completed height if the operator is frozen
-				record.CompleteBlockNumber = k.operatorKeeper.GetUnbondingExpirationBlockNumber(ctx, operatorAccAddress, record.BlockNumber)
-				if record.CompleteBlockNumber <= uint64(ctx.BlockHeight()) {
-					panic(fmt.Sprintf("the reset completedHeight isn't in future,setHeight:%v,curHeight:%v", record.CompleteBlockNumber, ctx.BlockHeight()))
-				}
-				_, innerError = k.SetSingleUndelegationRecord(ctx, record)
-				if innerError != nil {
-					panic(innerError)
-				}
-				continue
-			}*/
-
-		recordID := types.GetUndelegationRecordKey(record.BlockNumber, record.LzTxNonce, record.TxHash, record.OperatorAddr)
+		// TODO check if the operator has been slashed or frozen
+		recordID := types.GetUndelegationRecordKey(
+			record.BlockNumber, record.LzTxNonce, record.TxHash, record.OperatorAddr,
+		)
 		if k.GetUndelegationHoldCount(ctx, recordID) > 0 {
-			// delete it from state. then rewrite it with the next block
-			// #nosec G703 // the error is always nil
-			_ = k.DeleteUndelegationRecord(ctx, record)
-			// store it again with the next block and move on
+			// delete from all 3 states
+			if err := k.DeleteUndelegationRecord(ctx, record); err != nil {
+				logger.Error("failed to delete undelegation record", "error", err)
+				continue
+			}
+			// add back to all 3 states, with the new block height
 			// #nosec G701
 			record.CompleteBlockNumber = uint64(ctx.BlockHeight()) + 1
-			// we need to store two things here: one is the updated record in itself
-			// #nosec G703 // the error is always nil
-			recordKey, _ := k.SetSingleUndelegationRecord(ctx, record)
-			// and the other is the fact that it matures at the next block
-			// #nosec G703 // the error is always nil
-			_ = k.StorePendingUndelegationRecord(ctx, recordKey, record)
+			if err := k.SetUndelegationRecords(
+				ctx, []types.UndelegationRecord{*record},
+			); err != nil {
+				logger.Error("failed to set undelegation records", "error", err)
+				continue
+			}
 			writeCache()
 			continue
 		}
-		// TODO(mike): ensure that operator is required to perform self delegation to match above.
 
 		recordAmountNeg := record.Amount.Neg()
 		// update delegation state
 		deltaAmount := &types.DeltaDelegationAmounts{
 			WaitUndelegationAmount: recordAmountNeg,
 		}
-		_, err := k.UpdateDelegationState(ctx, record.StakerID, record.AssetID, record.OperatorAddr, deltaAmount)
-		if err != nil {
+		if _, err := k.UpdateDelegationState(
+			ctx, record.StakerID, record.AssetID, record.OperatorAddr, deltaAmount,
+		); err != nil {
 			// use oCtx so that the error is logged on the original context
-			k.Logger(oCtx).Error("failed to update delegation state", "error", err)
+			logger.Error(
+				"failed to update delegation state",
+				"error", err,
+			)
 			continue
 		}
 
 		// update the staker state
 		if record.AssetID == assetstypes.NativeAssetID {
-			parsedStakerID := strings.Split(record.StakerID, "_")
-			stakerAddr := sdk.AccAddress(hexutil.MustDecode(parsedStakerID[0]))
-			if err := k.bankKeeper.UndelegateCoinsFromModuleToAccount(ctx, types.DelegatedPoolName, stakerAddr, sdk.NewCoins(sdk.NewCoin(assetstypes.NativeAssetDenom, record.ActualCompletedAmount))); err != nil {
-				k.Logger(oCtx).Error("failed to undelegate coins from module to account", "error", err)
+			stakerAddrHex, _, err := assetstypes.ParseID(record.StakerID)
+			if err != nil {
+				logger.Error(
+					"failed to parse staker ID",
+					"error", err,
+				)
 				continue
 			}
-		} else {
-			err = k.assetsKeeper.UpdateStakerAssetState(ctx, record.StakerID, record.AssetID, assetstypes.DeltaStakerSingleAsset{
-				WithdrawableAmount:        record.ActualCompletedAmount,
-				PendingUndelegationAmount: recordAmountNeg,
-			})
+			stakerAddrBytes, err := hexutil.Decode(stakerAddrHex)
 			if err != nil {
-				k.Logger(oCtx).Error("failed to update staker asset state", "error", err)
+				logger.Error(
+					"failed to decode staker address",
+					"error", err,
+				)
+				continue
+			}
+			stakerAddr := sdk.AccAddress(stakerAddrBytes)
+			if err := k.bankKeeper.UndelegateCoinsFromModuleToAccount(
+				ctx, types.DelegatedPoolName, stakerAddr,
+				sdk.NewCoins(
+					sdk.NewCoin(assetstypes.NativeAssetDenom, record.ActualCompletedAmount),
+				),
+			); err != nil {
+				logger.Error(
+					"failed to undelegate coins from module to account",
+					"error", err,
+				)
+				continue
+			}
+		} else {
+			if err := k.assetsKeeper.UpdateStakerAssetState(
+				ctx, record.StakerID, record.AssetID,
+				assetstypes.DeltaStakerSingleAsset{
+					WithdrawableAmount:        record.ActualCompletedAmount,
+					PendingUndelegationAmount: recordAmountNeg,
+				},
+			); err != nil {
+				logger.Error(
+					"failed to update staker asset state",
+					"error", err,
+				)
 				continue
 			}
 		}
 
 		// update the operator state
-		err = k.assetsKeeper.UpdateOperatorAssetState(ctx, operatorAccAddress, record.AssetID, assetstypes.DeltaOperatorSingleAsset{
-			PendingUndelegationAmount: recordAmountNeg,
-		})
-		if err != nil {
-			k.Logger(oCtx).Error("failed to update operator asset state", "error", err)
+		if err := k.assetsKeeper.UpdateOperatorAssetState(
+			ctx, operatorAccAddress, record.AssetID,
+			assetstypes.DeltaOperatorSingleAsset{
+				PendingUndelegationAmount: recordAmountNeg,
+			},
+		); err != nil {
+			logger.Error(
+				"failed to update operator asset state",
+				"error", err,
+			)
 			continue
 		}
 
 		// delete the Undelegation records that have been complemented
-		// #nosec G703 // the error is always nil
-		_ = k.DeleteUndelegationRecord(ctx, record)
+		if err := k.DeleteUndelegationRecord(ctx, record); err != nil {
+			logger.Error(
+				"failed to delete undelegation record",
+				"error", err,
+			)
+			continue
+		}
 		// when calling `writeCache`, events are automatically emitted on the parent context
 		writeCache()
 	}

diff --git a/x/delegation/keeper/msg_server.go b/x/delegation/keeper/msg_server.go
@@ -13,18 +13,16 @@ import (
 
 var _ types.MsgServer = &Keeper{}
 
-// DelegateAssetToOperator todo: Delegation and Undelegation from exoCore chain directly will be implemented in future.At the moment,they are executed from client chain
-func (k *Keeper) DelegateAssetToOperator(goCtx context.Context, msg *types.MsgDelegation) (*types.DelegationResponse, error) {
+// DelegateAssetToOperator delegates asset to operator. Currently, it only supports native token
+func (k *Keeper) DelegateAssetToOperator(
+	goCtx context.Context, msg *types.MsgDelegation,
+) (*types.DelegationResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 	logger := k.Logger(ctx)
-	// TODO: currently we only support delegation with native token by invoking service
-	if msg.AssetID != assetstypes.NativeAssetID {
-		logger.Error("failed to delegate asset", "error", types.ErrNotSupportYet, "assetID", msg.AssetID)
-		return nil, types.ErrNotSupportYet.Wrap("assets other than native token are not supported yet")
-	}
-	logger.Info("DelegateAssetToOperator-nativeToken", "msg", msg)
-
+	// no need to validate whether assetID == native token, since that is done by ValidateBasic.
+	// we can use `Must` since pre-validated
 	fromAddr := sdk.MustAccAddressFromBech32(msg.BaseInfo.FromAddress)
+	// create nonce and unique hash
 	nonce, err := k.accountKeeper.GetSequence(ctx, fromAddr)
 	if err != nil {
 		logger.Error("failed to get nonce", "error", err)
@@ -35,30 +33,36 @@ func (k *Keeper) DelegateAssetToOperator(goCtx context.Context, msg *types.MsgDe
 	combined := fmt.Sprintf("%s-%d", txHash, nonce)
 	uniqueHash := sha256.Sum256([]byte(combined))
 
-	// test for refactor
-	delegationParamsList := newDelegationParams(msg.BaseInfo, assetstypes.NativeAssetAddr, assetstypes.NativeChainLzID, nonce, uniqueHash)
+	delegationParamsList := newDelegationParams(
+		msg.BaseInfo, assetstypes.NativeAssetAddr, assetstypes.NativeChainLzID,
+		nonce, uniqueHash,
+	)
+	cachedCtx, writeFunc := ctx.CacheContext()
 	for _, delegationParams := range delegationParamsList {
-		if err := k.DelegateTo(ctx, delegationParams); err != nil {
-			logger.Error("failed to delegate asset", "error", err, "delegationParams", delegationParams)
-			return &types.DelegationResponse{}, err
-
+		if err := k.DelegateTo(cachedCtx, delegationParams); err != nil {
+			logger.Error(
+				"failed to delegate asset",
+				"error", err,
+				"delegationParams", delegationParams,
+			)
+			return nil, err
 		}
 	}
-
+	writeFunc()
 	return &types.DelegationResponse{}, nil
 }
 
-func (k *Keeper) UndelegateAssetFromOperator(goCtx context.Context, msg *types.MsgUndelegation) (*types.UndelegationResponse, error) {
+// UndelegateAssetFromOperator undelegates asset from operator. Currently, it only supports
+// native token.
+func (k *Keeper) UndelegateAssetFromOperator(
+	goCtx context.Context, msg *types.MsgUndelegation,
+) (*types.UndelegationResponse, error) {
 	ctx := sdk.UnwrapSDKContext(goCtx)
 	logger := k.Logger(ctx)
-	// TODO: currently we only support undelegation with native token by invoking service
-	if msg.AssetID != assetstypes.NativeAssetID {
-		logger.Error("failed to undelegate asset", "error", types.ErrNotSupportYet, "assetID", msg.AssetID)
-		return nil, types.ErrNotSupportYet.Wrap("assets other than native token are not supported yet")
-	}
-	logger.Info("UndelegateAssetFromOperator", "msg", msg)
-
+	// can use `Must` since pre-validated
 	fromAddr := sdk.MustAccAddressFromBech32(msg.BaseInfo.FromAddress)
+	// no need to check that `assetID` is native token, since that is done by ValidateBasic.
+	// create nonce and unique hash
 	nonce, err := k.accountKeeper.GetSequence(ctx, fromAddr)
 	if err != nil {
 		logger.Error("failed to get nonce", "error", err)
@@ -69,19 +73,31 @@ func (k *Keeper) UndelegateAssetFromOperator(goCtx context.Context, msg *types.M
 	combined := fmt.Sprintf("%s-%d", txHash, nonce)
 	uniqueHash := sha256.Sum256([]byte(combined))
 
-	inputParamsList := newDelegationParams(msg.BaseInfo, assetstypes.NativeAssetAddr, assetstypes.NativeChainLzID, nonce, uniqueHash)
+	inputParamsList := newDelegationParams(
+		msg.BaseInfo, assetstypes.NativeAssetAddr, assetstypes.NativeChainLzID,
+		nonce, uniqueHash,
+	)
+	cachedCtx, writeFunc := ctx.CacheContext()
 	for _, inputParams := range inputParamsList {
-		if err := k.UndelegateFrom(ctx, inputParams); err != nil {
+		if err := k.UndelegateFrom(cachedCtx, inputParams); err != nil {
 			return nil, err
 		}
 	}
+	writeFunc()
 	return &types.UndelegationResponse{}, nil
 }
 
-func newDelegationParams(baseInfo *types.DelegationIncOrDecInfo, assetAddrStr string, clientChainLzID, txNonce uint64, txHash common.Hash) []*types.DelegationOrUndelegationParams {
+// newDelegationParams creates delegation params from the given base info.
+func newDelegationParams(
+	baseInfo *types.DelegationIncOrDecInfo,
+	assetAddrStr string, clientChainLzID uint64, txNonce uint64,
+	txHash common.Hash,
+) []*types.DelegationOrUndelegationParams {
+	// can use `Must` since pre-validated
 	stakerAddr := sdk.MustAccAddressFromBech32(baseInfo.FromAddress).Bytes()
 	res := make([]*types.DelegationOrUndelegationParams, 0, 1)
 	for _, kv := range baseInfo.PerOperatorAmounts {
+		// can use `Must` since pre-validated
 		operatorAddr := sdk.MustAccAddressFromBech32(kv.Key)
 		inputParams := types.NewDelegationOrUndelegationParams(
 			clientChainLzID,