Dockerfile (1)

Line range hint 26-30: Excellent security improvements and version update.

The changes in this segment are well-considered and align with Docker best practices:

1. Updating the curl package to version 8.9.1-r1 ensures you're using the latest patch version, which is good for security.
2. Creating a non-root user exocore and associated group is a crucial security practice for running containers.
3. Switching to the non-root user (UID 1000) before running the application further enhances security.
4. Changing the working directory to /home/exocore is logical and consistent with the new user setup.

These changes significantly improve the security posture of the container.

Consider adding a comment explaining the purpose of the UID/GID 1000, as it can help with file permissions when mounting volumes. For example:

# Create a non-root user with UID/GID 1000 for better volume mounting compatibility
RUN addgroup -g 1000 exocore \
    && adduser -S -h /home/exocore -D exocore -u 1000 -G exocore

x/assets/keeper/bank.go (2)

26-26: Approve the error handling improvement with a minor suggestion.

The change to use assetstypes.ErrInvalidDepositAmount.Wrapf instead of errorsmod.Wrapf is a good improvement. It provides a more specific error type, which can be helpful for error handling and logging.

Consider slightly modifying the error message for clarity:

-		return assetstypes.ErrInvalidDepositAmount.Wrapf("negative deposit amount:%s", params.OpAmount)
+		return assetstypes.ErrInvalidDepositAmount.Wrapf("deposit amount must be positive, got: %s", params.OpAmount)

This change makes the error message more explicit about the expected condition.

---

Line range hint 1-62: Consider addressing the TODO and improving function structure.

The function PerformDepositOrWithdraw handles its responsibilities well, but there are a couple of points to consider:

1. There's a TODO comment on line 52: "TODO: do we need additional process for exo-native-token ?" This should be addressed or removed if it's no longer relevant.

2. The function handles both deposit and withdrawal operations. Consider separating these into two distinct functions for improved readability and maintainability. This separation of concerns could make the code easier to understand and modify in the future.

Here's a suggested refactoring approach:

1. Create separate functions for deposit and withdrawal operations.
2. Use a factory pattern or simple conditional to call the appropriate function based on the params.Action.

This could look something like:

func (k Keeper) PerformDepositOrWithdraw(ctx sdk.Context, params *DepositWithdrawParams) error {
    switch params.Action {
    case assetstypes.DepositLST, assetstypes.DepositNST:
        return k.performDeposit(ctx, params)
    case assetstypes.WithdrawLST, assetstypes.WithdrawNST:
        return k.performWithdraw(ctx, params)
    default:
        return errorsmod.Wrapf(assetstypes.ErrInvalidOperationType, "the operation type is: %v", params.Action)
    }
}

func (k Keeper) performDeposit(ctx sdk.Context, params *DepositWithdrawParams) error {
    // Deposit-specific logic
}

func (k Keeper) performWithdraw(ctx sdk.Context, params *DepositWithdrawParams) error {
    // Withdrawal-specific logic
}

This structure could make the code more modular and easier to maintain.

x/delegation/types/msg.go (2)

84-89: LGTM: Improved function documentation

The added comment for validateDelegationInfo provides clear and concise information about the function's purpose and the validation checks it performs. This enhancement improves code maintainability and understanding.

Regarding the TODO note:

Consider creating a separate issue to track the potential improvement of using a single message with a flag for delegation/undelegation actions. This would help ensure the idea isn't lost and can be properly evaluated in the future.

---

96-98: LGTM: Improved error handling consistency

The updates to error handling in the validateDelegationInfo function improve consistency by using the Wrapf method. The reformatted error messages enhance readability while maintaining the original error checking logic.

Consider using constants for error messages to improve maintainability and consistency across the codebase. For example:

const (
    ErrMsgAmountNotPositive = "amount should be positive, got %s"
    ErrMsgOnlyNativeTokenSupported = "only nativeToken is supported, expected: %s, got: %s"
)

// Then use these constants in the error wrapping:
return ErrAmountIsNotPositive.Wrapf(ErrMsgAmountNotPositive, kv.Value.Amount.String())
return ErrInvalidAssetID.Wrapf(ErrMsgOnlyNativeTokenSupported, assetstype.ExocoreAssetID, assetID)

This approach would make it easier to maintain consistent error messages and potentially support internationalization in the future.

Also applies to: 102-104

x/delegation/keeper/msg_server.go (2)

43-55: Consider adding a comment about the caching context usage.

The introduction of a caching context is a good practice for ensuring atomic operations. Consider adding a brief comment explaining the purpose of the caching context to improve code readability and maintainability.

Suggested comment:

// Use a caching context to ensure all delegations are atomic
cachedCtx, writeFunc := ctx.CacheContext()

---

86-92: Consider adding a comment about the caching context usage.

The introduction of a caching context is a good practice for ensuring atomic operations. Consider adding a brief comment explaining the purpose of the caching context to improve code readability and maintainability.

Suggested comment:

// Use a caching context to ensure all undelegations are atomic
cachedCtx, writeFunc := ctx.CacheContext()

precompiles/assets/IAssets.sol (2)

17-18: Improved clarity in function documentation.

The changes in the comment for depositLST function provide better clarity about its purpose and where the state changes occur. The specification that it's "mainly LSTs" adds useful information about the types of assets being deposited.

Consider adding a brief explanation or expanding the acronym LST (Liquid Staking Token) for improved readability, especially for developers who might not be familiar with the term.

---

Line range hint 66-66: Inconsistency in withdrawNST function documentation.

The comment for the withdrawNST function still mentions the "withdraw module", which is inconsistent with the updates made to other functions in this interface.

To maintain consistency, consider updating the comment as follows:

-    /// @dev withdraw NST To the staker, that will change the state in withdraw module
+    /// @dev withdraw NST To the staker, that will change the state in assets module

This change will align the withdrawNST function documentation with the other updated functions in this interface.

precompiles/assets/tx.go (1)

193-194: Approve changes with a suggestion for improved documentation.

The simplification of the asset update logic is a good improvement. It reduces code complexity and potentially improves performance by combining the existence check and metadata update into a single operation.

Consider updating the comment to more accurately reflect the function's behavior:

-	// this verifies the existence of the asset and returns an error if it doesn't exist
+	// this verifies the existence of the asset, updates its metadata if it exists, and returns an error otherwise

This change would provide a more complete description of what the function does, improving code clarity for future maintainers.

x/avs/keeper/avs.go (1)

Line range hint 86-97: Approved: Early return improves function efficiency

The addition of the early return statement return true // stop because we found the AVS at line 93 is a good optimization. It stops the iteration as soon as the matching AVS is found, preventing unnecessary iterations through the remaining AVS entries.

While this change improves the function's efficiency, consider addressing the TODO comment in the future. Implementing a reverse mapping from task addresses to AVS info could significantly enhance performance, especially if this function is frequently called with a large number of AVS entries.

x/delegation/keeper/abci.go (2)

33-33: Address the TODO: Check if the operator has been slashed or frozen

There's a TODO comment at line 33 indicating that a check needs to be implemented to verify if the operator has been slashed or frozen. Implementing this check is important to ensure correct handling of operators in these states.

Would you like assistance in implementing this check or opening a GitHub issue to track this task?

---

130-130: Typographical error in comment: 'complemented' should be 'completed'

In the comment at line 130, the word "complemented" is likely a typo and should be "completed".

x/assets/keeper/client_chain_asset.go (1)

Line range hint 94-108: Refactor GetAssetsDecimal function parameter for clarity and efficiency

The GetAssetsDecimal function accepts assets map[string]interface{} but only uses the keys. Consider changing the parameter to assetIDs []string for clarity and to avoid unnecessary use of an empty interface.

Apply this diff to adjust the function signature and implementation:

-func (k Keeper) GetAssetsDecimal(ctx sdk.Context, assets map[string]interface{}) (decimals map[string]uint32, err error) {
-    if assets == nil {
-        return nil, errorsmod.Wrap(assetstype.ErrInputPointerIsNil, "assets is nil")
-    }
+func (k Keeper) GetAssetsDecimal(ctx sdk.Context, assetIDs []string) (decimals map[string]uint32, err error) {
+    if assetIDs == nil {
+        return nil, errorsmod.Wrap(assetstype.ErrInputPointerIsNil, "assetIDs is nil")
     }
     store := prefix.NewStore(ctx.KVStore(k.storeKey), assetstype.KeyPrefixReStakingAssetInfo)
-    decimals = make(map[string]uint32, 0)
-    for assetID := range assets {
+    decimals = make(map[string]uint32, len(assetIDs))
+    for _, assetID := range assetIDs {
         value := store.Get([]byte(assetID))
         if value == nil {
             return nil, assetstype.ErrNoClientChainAssetKey
         }
         ret := assetstype.StakingAssetInfo{}
         k.cdc.MustUnmarshal(value, &ret)
         decimals[assetID] = ret.AssetBasicInfo.Decimals
     }
 
     return decimals, nil
 }

x/delegation/keeper/un_delegation_state.go (7)

33-36: Improve comment formatting for storage keys

The comment explaining how undelegation records are stored can be reformatted for better readability. Consider formatting the keys using bullet points or code blocks to enhance clarity.

You might revise it as:

// The records are stored with 3 different keys:
// (1) recordKey == blockNumber + lzNonce + txHash + operatorAddress => record
// (2) stakerID + assetID + lzNonce => recordKey
// (3) completeBlockNumber + lzNonce => recordKey

---

38-38: Address TODO comment in SetUndelegationRecords

There's a TODO comment indicating uncertainty:

// todo: check if the following state can only be set once?

Consider resolving this TODO or creating a GitHub issue to track it.

Would you like me to open a GitHub issue to help address this TODO item?

---

Line range hint 81-92: Optimize error handling in GetUndelegationRecords

When retrieving records, if a key doesn't exist in the store, the function returns an error and halts. If multiple keys are provided and some are missing, this could prevent valid records from being returned.

Consider handling missing keys gracefully:

• Accumulate valid records and continue processing even if some keys are missing.
• Collect missing keys and return a consolidated error message at the end.

This approach ensures that all available records are returned, and the caller is informed about any missing ones.

---

Line range hint 158-176: Handle missing undelegation records in IterateUndelegationsByStakerAndAsset

In this function, if an undelegation record key exists in the KeyPrefixStakerUndelegationInfo store but the corresponding record in the KeyPrefixUndelegationInfo store is missing, an error is returned, halting the iteration.

Consider handling this scenario to prevent unexpected termination:

• Log a warning message about the missing record.
• Decide if the absence indicates a data inconsistency that needs addressing.
• Continue the iteration to process remaining records.

This ensures robustness in the presence of partial data inconsistencies.

---

Line range hint 205-215: Ensure consistent return values in GetPendingUndelegationRecords

When no record keys are found, the function initializes records as an empty slice and returns it. To maintain consistency and prevent potential nil dereferences in calling code, ensure that an empty slice is always returned instead of nil.

This small adjustment enhances reliability for functions that iterate over the returned records without needing to check for nil.

---

Line range hint 220-228: Ensure atomicity in IncrementUndelegationHoldCount

If multiple processes or threads might increment the hold count concurrently, there's a risk of race conditions leading to inaccurate counts.

Consider implementing synchronization mechanisms or utilizing atomic operations to ensure that the hold count remains accurate in concurrent environments.

• Use mutexes or locks if the SDK supports them.
• Explore transactional KVStore operations if available.

This will prevent inconsistent state due to concurrent modifications.

---

Line range hint 220-228: Refactor to reduce code duplication in hold count functions

The IncrementUndelegationHoldCount and DecrementUndelegationHoldCount functions have similar logic with minor differences.

Consider creating a helper function to update the hold count:

func (k Keeper) updateUndelegationHoldCount(ctx sdk.Context, recordKey []byte, delta int64) error {
	prev := k.GetUndelegationHoldCount(ctx, recordKey)

	var now uint64
	if delta > 0 {
		if prev > math.MaxUint64-uint64(delta) {
			return types.ErrCannotIncHoldCount
		}
		now = prev + uint64(delta)
	} else {
		if prev < uint64(-delta) {
			return types.ErrCannotDecHoldCount
		}
		now = prev - uint64(-delta)
	}

	store := ctx.KVStore(k.storeKey)
	store.Set(types.GetUndelegationOnHoldKey(recordKey), sdk.Uint64ToBigEndian(now))
	return nil
}

Then, refactor the increment and decrement functions:

func (k Keeper) IncrementUndelegationHoldCount(ctx sdk.Context, recordKey []byte) error {
	return k.updateUndelegationHoldCount(ctx, recordKey, 1)
}

func (k Keeper) DecrementUndelegationHoldCount(ctx sdk.Context, recordKey []byte) error {
	return k.updateUndelegationHoldCount(ctx, recordKey, -1)
}

This reduces code duplication and simplifies maintenance.

Configuration used: CodeRabbit UI
Review profile: CHILL

x/assets/types/errors.go (1)

91-93: Consider enhancing the error message for clarity.

The new error ErrRegisterDuplicateAssetID is well-defined and consistent with other errors in the file. However, the error message could be more descriptive to provide better context.

Consider updating the error message to be more specific:

 ErrRegisterDuplicateAssetID = errorsmod.Register(
 	ModuleName, 19,
-	"register new asset with an existing assetID")
+	"attempt to register a new asset with an already existing assetID")

This change makes the error message more explicit about the nature of the error, which can aid in debugging and error handling.

x/assets/keeper/client_chain_asset.go (2)

52-54: Consider adding spaces after commas in the error message for better readability

The error message could be more readable by adding spaces after commas.

Apply this diff to improve the error message formatting:

-            "the asset has already been registered,assetID:%v,LayerZeroChainID:%v,ClientChainAssetAddr:%v",
+            "the asset has already been registered, assetID:%v, LayerZeroChainID:%v, ClientChainAssetAddr:%v",

---

Line range hint 94-109: Consider changing the assets parameter type for clarity

The assets parameter is currently of type map[string]interface{}, but only the keys are used in the function. Consider changing it to []string or map[string]struct{} to indicate a set of asset IDs. This can improve code readability and intent.

Apply this diff to use []string for assets:

-func (k Keeper) GetAssetsDecimal(ctx sdk.Context, assets map[string]interface{}) (decimals map[string]uint32, err error) {
+func (k Keeper) GetAssetsDecimal(ctx sdk.Context, assets []string) (decimals map[string]uint32, err error) {
     if assets == nil {
         return nil, errorsmod.Wrap(assetstype.ErrInputPointerIsNil, "assets is nil")
     }
     store := prefix.NewStore(ctx.KVStore(k.storeKey), assetstype.KeyPrefixReStakingAssetInfo)
     decimals = make(map[string]uint32)
-    for assetID := range assets {
+    for _, assetID := range assets {
         value := store.Get([]byte(assetID))
         if value == nil {
             return nil, assetstype.ErrNoClientChainAssetKey
         }
         ret := assetstype.StakingAssetInfo{}
         k.cdc.MustUnmarshal(value, &ret)
         decimals[assetID] = ret.AssetBasicInfo.Decimals
     }
     return decimals, nil
 }

Configuration used: CodeRabbit UI
Review profile: CHILL