fix: return error for non-existent states + unit tests

cairo-contracts/packages/apps/src/tests/transfer.cairo

@@ -1,15 +1,19 @@
+use TokenTransferComponent::TransferValidationTrait;
 use openzeppelin_testing::events::EventSpyExt;
 use snforge_std::cheatcodes::events::EventSpy;
 use snforge_std::spy_events;
+use starknet::class_hash::class_hash_const;
 use starknet_ibc_apps::transfer::ERC20Contract;
 use starknet_ibc_apps::transfer::TokenTransferComponent::{
-    TransferInitializerImpl, TransferReaderImpl
+    TransferInitializerImpl, TransferReaderImpl, TransferWriterImpl, IBCTokenAddress
 };
 use starknet_ibc_apps::transfer::TokenTransferComponent;
 use starknet_ibc_core::router::{AppContract, AppContractTrait};
 use starknet_ibc_testkit::configs::{TransferAppConfigTrait, TransferAppConfig};
 use starknet_ibc_testkit::dummies::CLASS_HASH;
-use starknet_ibc_testkit::dummies::{SUPPLY, OWNER, NAME, SYMBOL, COSMOS, STARKNET};
+use starknet_ibc_testkit::dummies::{
+    AMOUNT, SUPPLY, OWNER, NAME, SYMBOL, COSMOS, STARKNET, HOSTED_DENOM, EMPTY_MEMO
+};
 use starknet_ibc_testkit::event_spy::TransferEventSpyExt;
 use starknet_ibc_testkit::handles::{ERC20Handle, AppHandle};
 use starknet_ibc_testkit::mocks::MockTransferApp;
@@ -48,6 +52,29 @@ fn test_init_state() {
     assert_eq!(class_hash, CLASS_HASH());
 }
 
+#[test]
+#[should_panic(expected: 'ICS20: erc20 class hash is 0')]
+fn test_missing_class_hash() {
+    let mut state = setup_component();
+    state.write_erc20_class_hash(class_hash_const::<0>());
+    state.read_erc20_class_hash();
+}
+
+#[test]
+#[should_panic(expected: 'ICS20: salt is 0')]
+fn test_missing_salt() {
+    let mut state = setup_component();
+    state.write_salt(0);
+    state.read_salt();
+}
+
+#[test]
+#[should_panic(expected: 'ICS20: missing token address')]
+fn test_missing_ibc_token_address() {
+    let state = setup_component();
+    state.ibc_token_address(0);
+}
+
 #[test]
 fn test_escrow_ok() {
     let (ics20, mut erc20, cfg, mut spy) = setup();
@@ -112,7 +139,7 @@ fn test_mint_ok() {
 
     let prefixed_denom = cfg.prefix_hosted_denom();
 
-    let token_address = ics20.ibc_token_address(prefixed_denom.key()).unwrap();
+    let token_address = ics20.ibc_token_address(prefixed_denom.key());
 
     // Assert the `CreateTokenEvent` emitted.
     spy.assert_create_token_event(ics20.address, NAME(), SYMBOL(), token_address, cfg.amount);
@@ -154,7 +181,7 @@ fn test_burn_ok() {
 
     let prefixed_denom = cfg.prefix_hosted_denom();
 
-    let token_address = ics20.ibc_token_address(prefixed_denom.key()).unwrap();
+    let token_address = ics20.ibc_token_address(prefixed_denom.key());
 
     let erc20: ERC20Contract = token_address.into();
 
@@ -176,3 +203,10 @@ fn test_burn_ok() {
     // Chekck the total supply of the ERC20 contract.
     erc20.assert_total_supply(0);
 }
+
+#[test]
+#[should_panic(expected: 'ICS20: missing token address')]
+fn test_burn_non_existence_ibc_token() {
+    let state = setup_component();
+    state.burn_validate(OWNER(), HOSTED_DENOM(), AMOUNT, EMPTY_MEMO());
+}

cairo-contracts/packages/apps/src/transfer/components/transfer.cairo

@@ -116,7 +116,7 @@ pub mod TokenTransferComponent {
 
             self.write_erc20_class_hash(erc20_class_hash);
 
-            self.write_salt(0);
+            self.write_salt(1);
         }
     }
 
@@ -203,14 +203,12 @@ pub mod TokenTransferComponent {
     > of ITokenAddress<ComponentState<TContractState>> {
         fn ibc_token_address(
             self: @ComponentState<TContractState>, token_key: felt252
-        ) -> Option<ContractAddress> {
-            let token_address = self.read_ibc_token_address(token_key);
+        ) -> ContractAddress {
+            let address = self.read_ibc_token_address(token_key);
 
-            if token_address.is_non_zero() {
-                Option::Some(token_address)
-            } else {
-                Option::None
-            }
+            assert(address.is_non_zero(), TransferErrors::ZERO_TOKEN_ADDRESS);
+
+            address
         }
     }
 
@@ -499,6 +497,8 @@ pub mod TokenTransferComponent {
         ) {
             let token = self.get_token(denom.key());
 
+            assert(token.is_non_zero(), TransferErrors::ZERO_TOKEN_ADDRESS);
+
             let balance = token.balance_of(account);
 
             assert(balance >= amount, TransferErrors::INSUFFICIENT_BALANCE);
@@ -691,13 +691,26 @@ pub mod TokenTransferComponent {
         TContractState, +HasComponent<TContractState>, +Drop<TContractState>
     > of TransferReaderTrait<TContractState> {
         fn read_erc20_class_hash(self: @ComponentState<TContractState>) -> ClassHash {
-            self.erc20_class_hash.read()
+            let class_hash = self.erc20_class_hash.read();
+
+            assert(class_hash.is_non_zero(), TransferErrors::ZERO_ERC20_CLASS_HASH);
+
+            class_hash
         }
 
         fn read_salt(self: @ComponentState<TContractState>) -> felt252 {
-            self.salt.read()
+            let salt = self.salt.read();
+
+            assert(salt.is_non_zero(), TransferErrors::ZERO_SALT);
+
+            salt
         }
 
+        // NOTE: The `read_ibc_token_address` and `read_ibc_token_key` methods
+        // do not reject cases where the value might be zero (non-existent). As
+        // these methods are also called internally where there is logic for
+        // handling non-existent cases.
+
         fn read_ibc_token_address(
             self: @ComponentState<TContractState>, token_key: felt252
         ) -> ContractAddress {

cairo-contracts/packages/apps/src/transfer/errors.cairo

@@ -4,6 +4,8 @@ pub mod TransferErrors {
     pub const ZERO_OWNER: felt252 = 'ICS20: owner is 0';
     pub const ZERO_ERC20_CLASS_HASH: felt252 = 'ICS20: erc20 class hash is 0';
     pub const ZERO_AMOUNT: felt252 = 'ICS20: transfer amount is 0';
+    pub const ZERO_SALT: felt252 = 'ICS20: salt is 0';
+    pub const ZERO_TOKEN_ADDRESS: felt252 = 'ICS20: missing token address';
     pub const INVALID_DENOM: felt252 = 'ICS20: invalid denom';
     pub const INVALID_PACKET_DATA: felt252 = 'ICS20: invalid packet data';
     pub const INVALID_OWNER: felt252 = 'ICS20: invalid owner';

cairo-contracts/packages/apps/src/transfer/interfaces/transfer.cairo

@@ -24,6 +24,6 @@ pub trait ITokenAddress<TContractState> {
     /// }
     /// ```
     /// Hashing the denom is delegated to the client as it is more cost-efficient.
-    fn ibc_token_address(self: @TContractState, token_key: felt252) -> Option<ContractAddress>;
+    fn ibc_token_address(self: @TContractState, token_key: felt252) -> ContractAddress;
 }
 

cairo-contracts/packages/clients/src/cometbft/client_state.cairo

@@ -1,5 +1,6 @@
+use core::num::traits::Zero;
 use starknet_ibc_clients::cometbft::CometErrors;
-use starknet_ibc_core::client::{Height, HeightPartialOrd, Status};
+use starknet_ibc_core::client::{Height, HeightPartialOrd, Status, StatusTrait};
 
 #[derive(Clone, Debug, Drop, Hash, PartialEq, Serde, starknet::Store)]
 pub struct CometClientState {
@@ -10,6 +11,12 @@ pub struct CometClientState {
 
 #[generate_trait]
 pub impl CometClientStateImpl of CometClientStateTrait {
+    fn is_non_zero(self: @CometClientState) -> bool {
+        !(self.latest_height.is_zero()
+            && self.trusting_period.is_zero()
+            && self.status.is_expired())
+    }
+
     fn deserialize(client_state: Array<felt252>,) -> CometClientState {
         let mut client_state_span = client_state.span();
 

cairo-contracts/packages/clients/src/cometbft/component.cairo

@@ -1,5 +1,6 @@
 #[starknet::component]
 pub mod CometClientComponent {
+    use core::num::traits::Zero;
     use starknet::storage::{
         Map, StorageMapReadAccess, StorageMapWriteAccess, StoragePointerReadAccess,
         StoragePointerWriteAccess
@@ -91,8 +92,6 @@ pub mod CometClientComponent {
             let latest_consensus_state = self
                 .read_consensus_state(client_sequence, comet_client_state.latest_height.clone());
 
-            assert(!latest_consensus_state.is_zero(), CometErrors::ZERO_CONSENSUS_STATE);
-
             self._status(comet_client_state, latest_consensus_state, client_sequence)
         }
 
@@ -168,8 +167,6 @@ pub mod CometClientComponent {
             let latest_consensus_state = self
                 .read_consensus_state(client_sequence, comet_client_state.latest_height.clone());
 
-            assert(!latest_consensus_state.is_zero(), CometErrors::ZERO_CONSENSUS_STATE);
-
             let status = self
                 ._status(comet_client_state, latest_consensus_state, msg.client_id.sequence);
 
@@ -295,10 +292,7 @@ pub mod CometClientComponent {
 
             // TODO: Implement consensus state pruning mechanism.
 
-            let maybe_consensus_state = self
-                .read_consensus_state(client_sequence, header_height.clone());
-
-            if maybe_consensus_state.is_zero() {
+            if !self.consensus_state_exists(client_sequence, header_height.clone()) {
                 let mut client_state = self.read_client_state(client_sequence);
 
                 client_state.update(header_height.clone());
@@ -353,11 +347,7 @@ pub mod CometClientComponent {
         fn _root(self: @ComponentState<TContractState>, client_sequence: u64) -> ByteArray {
             let latest_height = self.latest_height(client_sequence);
 
-            let latest_consensus_state = self.read_consensus_state(client_sequence, latest_height);
-
-            assert(!latest_consensus_state.is_zero(), CometErrors::ZERO_CONSENSUS_STATE);
-
-            latest_consensus_state.root
+            self.read_consensus_state(client_sequence, latest_height).root
         }
 
         fn _status(
@@ -427,25 +417,47 @@ pub mod CometClientComponent {
         fn read_client_state(
             self: @ComponentState<TContractState>, client_sequence: u64
         ) -> CometClientState {
-            self.client_states.read(client_sequence)
+            let client_state = self.client_states.read(client_sequence);
+
+            assert(client_state.is_non_zero(), CometErrors::MISSING_CLIENT_STATE);
+
+            client_state
         }
 
         fn read_consensus_state(
             self: @ComponentState<TContractState>, client_sequence: u64, height: Height
         ) -> CometConsensusState {
-            self.consensus_states.read((client_sequence, height))
+            let consensus_state = self.consensus_states.read((client_sequence, height));
+
+            assert(consensus_state.is_non_zero(), CometErrors::MISSING_CONSENSUS_STATE);
+
+            consensus_state
+        }
+
+        fn consensus_state_exists(
+            self: @ComponentState<TContractState>, client_sequence: u64, height: Height
+        ) -> bool {
+            self.consensus_states.read((client_sequence, height)).is_non_zero()
         }
 
         fn read_client_processed_time(
             self: @ComponentState<TContractState>, client_sequence: u64, height: Height
         ) -> Timestamp {
-            self.client_processed_times.read((client_sequence, height)).into()
+            let processed_time = self.client_processed_times.read((client_sequence, height));
+
+            assert(processed_time.is_non_zero(), CometErrors::MISSING_CLIENT_PROCESSED_TIME);
+
+            processed_time.into()
         }
 
         fn read_client_processed_height(
             self: @ComponentState<TContractState>, client_sequence: u64, height: Height
         ) -> u64 {
-            self.client_processed_heights.read((client_sequence, height))
+            let processed_height = self.client_processed_heights.read((client_sequence, height));
+
+            assert(processed_height.is_non_zero(), CometErrors::MISSING_CLIENT_PROCESSED_HEIGHT);
+
+            processed_height
         }
     }
 

cairo-contracts/packages/clients/src/cometbft/consensus_state.cairo

@@ -10,8 +10,8 @@ pub struct CometConsensusState {
 
 #[generate_trait]
 pub impl CometConsensusStateImpl of CometConsensusStateTrait {
-    fn is_zero(self: @CometConsensusState) -> bool {
-        self.root.len() == 0 && self.timestamp.is_zero()
+    fn is_non_zero(self: @CometConsensusState) -> bool {
+        !(self.root.len() == 0 && self.timestamp.is_zero())
     }
 
     fn timestamp(self: @CometConsensusState) -> u64 {

cairo-contracts/packages/clients/src/cometbft/errors.cairo

@@ -5,5 +5,8 @@ pub mod CometErrors {
     pub const INVALID_CONSENSUS_STATE: felt252 = 'ICS07: invalid consensus state';
     pub const INVALID_HEADER: felt252 = 'ICS07: invalid header';
     pub const INVALID_HEADER_TIMESTAMP: felt252 = 'ICS07: invalid header timestamp';
-    pub const ZERO_CONSENSUS_STATE: felt252 = 'ICS07: zero consensus state';
+    pub const MISSING_CLIENT_STATE: felt252 = 'ICS07: missing client state';
+    pub const MISSING_CONSENSUS_STATE: felt252 = 'ICS07: missing consensus state';
+    pub const MISSING_CLIENT_PROCESSED_TIME: felt252 = 'ICS07: missing processed time';
+    pub const MISSING_CLIENT_PROCESSED_HEIGHT: felt252 = 'ICS07: missing processed height';
 }

cairo-contracts/packages/clients/src/tests/cometbft.cairo

@@ -60,3 +60,31 @@ fn test_update_client_ok() {
     assert_eq!(state.latest_height(0), updating_height);
     assert!(state.status(0).is_active());
 }
+
+#[test]
+#[should_panic(expected: 'ICS07: missing client state')]
+fn test_missing_client_state() {
+    let mut state = setup();
+    state.read_client_state(0);
+}
+
+#[test]
+#[should_panic(expected: 'ICS07: missing consensus state')]
+fn test_missing_consensus_state() {
+    let mut state = setup();
+    state.read_consensus_state(0, HEIGHT(5));
+}
+
+#[test]
+#[should_panic(expected: 'ICS07: missing processed time')]
+fn test_missing_client_processed_time() {
+    let mut state = setup();
+    state.read_client_processed_time(0, HEIGHT(5));
+}
+
+#[test]
+#[should_panic(expected: 'ICS07: missing processed height')]
+fn test_missing_client_processed_height() {
+    let mut state = setup();
+    state.read_client_processed_height(0, HEIGHT(5));
+}