Some Nits and Issues on the Dex adapter and variable naming #240

Activity

1. cyberhoward commented on Feb 8, 2024

   

   cyberhoward

   on Feb 8, 2024

   Contributor

   More actions

   1. and 2. are very good feedback.

   2. is because of IBC. if an action is performed "over IBC" then the adapter should return an ibc callback with that ID. At least that was the idea. I think the IBC aspect of these adapters needs a major revamp, but we can do that when we get there.

   3. Yes, good spot!
2. kaka-acml commented on Feb 23, 2025

   

   kaka-acml

   on Feb 23, 2025

   More actions

   Your Concern: In abstract/framework/packages/standards/dex/src/command.rs, the DexCommand trait uses a generic error type E defaulting to DexError:
   rust
   pub trait DexCommand<E: Error = DexError>: Identify {
   You’d prefer a concrete type like DexError instead of a generic.
3. Natasha-cyber777 commented on Jun 23, 2025

   

   Natasha-cyber777

   on Jun 23, 2025

   More actions

   I agree with this. While generics offer flexibility, if DexError is designed to be the comprehensive error type for all DexCommand implementations within our framework, making it a concrete associated type or directly using DexError simplifies the trait and makes the error handling more explicit and less verbose for implementers.
   Remove the generic E and instead use DexError directly. This assumes DexError is indeed the canonical error type for DEX operations.
   pub trait DexCommand: Identify {
   fn provide_liquidity(
   ) -> Result<ContractResponse, DexError>;
   }

   This simplifies the trait definition and makes the error type explicit. If there's a reason for implementers to use their own specific error types that convert into DexError, then an associated type Error might still be useful, but the generic default E with DexError fallback is often a source of confusion. Given the context, directly using DexError seems like the cleaner approach.

   Consistency in naming, especially when interacting with well-established patterns from libraries like cw_asset, significantly improves readability, reduces cognitive load, and makes the code more intuitive for anyone familiar with the CosmWasm ecosystem. Aligning our naming with cw_asset's Asset and AssetInfo pattern while keeping our Ans prefix makes perfect sense.
   Rename AssetEntry to AnsAssetInfo:
   pub struct AnsAsset {
   pub info: AnsAssetInfo,
   pub amount: Uint128,
   }
   pub enum AnsAssetInfo {
   // ...
   }

   This change would propagate throughout the code where AssetEntry is used.

   Modify the Identify trait definition to require Default.
   use std::default::Default;

   pub trait Identify: Default {
   // ...
   }
   With this change, any type that implements Identify will automatically be required by the Rust compiler to also implement Default. This ensures consistency and simplifies any code that relies on being able to create a default instance of an Identify-able type.