General Comments

• The implementation appears robust, with thorough error handling and strong security measures in place.
• Comprehensive test modules are provided, following DeFi best practices to identify and address potential edge cases.
• Key test functions are publicly exposed, enabling seamless integration of FullSail infrastructure modules with other smart contracts.

FullSail-CLMM-SC

• Implementation of concentrated liquidity market maker

Recommendations

• pool.move

  • line 998

  assert!(integer_mate::math_u128::add_check(pool.liquidity, liquidity), EInsufficientLiquidity);
  pool.liquidity = pool.liquidity + liquidity;
  

  • the overhead of calling add_check before every addition could add up

    • call integer_mate::math_u128::overflowing_add instead, which might be more efficient as it handles overflow detection and computation in one step

  • line 3505

  assert!(integer_mate::math_u128::add_check(pool.liquidity, liquidity), EInsufficientLiquidity);
  pool.liquidity = pool.liquidity + liquidity;
  

  • Requires last_updated == now to sync
    • change to >=

• linked_table.move

  • seems to be a well-designed and correct implementation of a doubly-link list

  • suitable for CLMM for managing ticks, liquidity positions, and oSAIL lockups

  • one recommendation is to clarify how LinkedTable is used to guide developers

  • add error checks

    • check for key existence in borrow, insert_before, inser_after:

    const EKeyNotFound: u64 = 1;
    assert!(contains(table, current_key), EKeyNotFound);
    

    • prevent duplicate keys in push_front, push_back:

    const EKeyAlreadyExists: u64 = 2;
    assert!(!contains(table, key), EKeyAlreadyExists);
    

• config.move

  • constant VERSION could be used just like in other modules

FullSail-LL-SC

• liquidity_lock_v1.move

  • line 539

  check_admin(locker, sui::tx_context::sender(ctx));
  

  • Is this check necessary? The function already requires a SuperAdminCap so maybe it’s not needed?

  • How about emitting events whenever admin related functionalities are updated?

  • line 941

  locker: &mut Locker,
  

  • &mut Locker is not needed; consider changing to &Locker

  • line 1891

  public fun collect_fee<CoinTypeA, CoinTypeB>(
  

  • collect_fee is allowed during pause but other state-changing functions are not allowed. If it is ok by design, maybe document it for future references