NDEV-2505: Code review suggestions (#254)

* Remove unused AccountStorage::code_hash method

* Replace Buffer::buffer_is_empty with [T]::is_empty

* Rename from_address to address (consistent with trait)

* Rename buffer to code

* Inline data_account_exists method

* Remove TODOs

* Reformat

* Add test_keccak_hash_empty_slice

* Add Buffer::is_empty tests

* Use early return to simplify code flow

evm_loader/lib/src/account_storage.rs

@@ -623,27 +623,6 @@ impl<T: Rpc> AccountStorage for EmulatorAccountStorage<'_, T> {
         address.find_solana_address(self.program_id())
     }
 
-    async fn code_hash(&self, address: Address, chain_id: u64) -> [u8; 32] {
-        use solana_sdk::keccak::hash;
-
-        info!("code_hash {address} {chain_id}");
-
-        let code = self.code(address).await.to_vec();
-        if !code.is_empty() {
-            return hash(&code).to_bytes();
-        }
-
-        // https://eips.ethereum.org/EIPS/eip-1052
-        // https://eips.ethereum.org/EIPS/eip-161
-        if (self.balance(address, chain_id).await == 0)
-            && (self.nonce(address, chain_id).await == 0)
-        {
-            return <[u8; 32]>::default();
-        }
-
-        hash(&[]).to_bytes()
-    }
-
     async fn code_size(&self, address: Address) -> usize {
         info!("code_size {address}");
 

evm_loader/program/src/account_storage/backend.rs

@@ -73,29 +73,6 @@ impl<'a> AccountStorage for ProgramAccountStorage<'a> {
             .contract_with_bump_seed(self.program_id(), address)
     }
 
-    fn code_hash(&self, address: Address, chain_id: u64) -> [u8; 32] {
-        use solana_program::keccak;
-
-        if let Ok(contract) = self.contract_account(address) {
-            keccak::hash(&contract.code()).to_bytes()
-        } else {
-            // https://eips.ethereum.org/EIPS/eip-1052
-            // https://eips.ethereum.org/EIPS/eip-161
-            if let Ok(account) = self.balance_account(address, chain_id) {
-                if account.nonce() > 0 || account.balance() > 0 {
-                    // account without code
-                    keccak::hash(&[]).to_bytes()
-                } else {
-                    // non-existent account
-                    <[u8; 32]>::default()
-                }
-            } else {
-                // non-existent account
-                <[u8; 32]>::default()
-            }
-        }
-    }
-
     fn code_size(&self, address: Address) -> usize {
         self.contract_account(address).map_or(0, |a| a.code_len())
     }

evm_loader/program/src/account_storage/mod.rs

@@ -58,8 +58,6 @@ pub trait AccountStorage {
     /// Get contract solana address
     fn contract_pubkey(&self, address: Address) -> (Pubkey, u8);
 
-    /// Get code hash
-    async fn code_hash(&self, address: Address, chain_id: u64) -> [u8; 32];
     /// Get code size
     async fn code_size(&self, address: Address) -> usize;
     /// Get code data

evm_loader/program/src/evm/buffer.rs

@@ -83,11 +83,6 @@ impl Buffer {
         Buffer::new(Inner::Empty)
     }
 
-    #[must_use]
-    pub fn buffer_is_empty(&self) -> bool {
-        matches!(self.inner, Inner::Empty)
-    }
-
     #[must_use]
     pub fn is_initialized(&self) -> bool {
         !matches!(self.inner, Inner::AccountUninit { .. })
@@ -240,3 +235,47 @@ impl<'de> serde::Deserialize<'de> for Buffer {
         deserializer.deserialize_enum("evm_buffer", &["empty", "owned", "account"], BufferVisitor)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::cell::RefCell;
+    use std::rc::Rc;
+
+    #[test]
+    fn test_empty_buffer_is_empty() {
+        assert!(Buffer::empty().is_empty());
+    }
+
+    #[test]
+    fn test_owned_buffer_is_empty() {
+        assert!(!Buffer::from_vec(vec![0]).is_empty());
+    }
+
+    #[test]
+    fn test_account_buffer_is_empty() {
+        let mut lamports = 0;
+        let mut data = [0];
+        let account_info = AccountInfo {
+            key: &Default::default(),
+            lamports: Rc::new(RefCell::new(&mut lamports)),
+            data: Rc::new(RefCell::new(&mut data)),
+            owner: &Default::default(),
+            rent_epoch: 0,
+            is_signer: false,
+            is_writable: false,
+            executable: false,
+        };
+        assert!(!(unsafe { Buffer::from_account(&account_info, 0..1) }).is_empty());
+    }
+
+    #[test]
+    #[should_panic(expected = "assertion failed: !self.ptr.is_null()")]
+    fn test_account_uninit_buffer_is_empty() {
+        assert!(!Buffer::new(Inner::AccountUninit {
+            key: Default::default(),
+            range: Default::default(),
+        })
+        .is_empty());
+    }
+}

evm_loader/program/src/executor/state.rs

@@ -4,6 +4,7 @@ use std::collections::BTreeMap;
 use ethnum::{AsU256, U256};
 use maybe_async::maybe_async;
 use solana_program::instruction::Instruction;
+use solana_program::keccak;
 use solana_program::pubkey::Pubkey;
 
 use crate::account_storage::AccountStorage;
@@ -284,40 +285,23 @@ impl<'a, B: AccountStorage> Database for ExecutorState<'a, B> {
         Ok(self.backend.code_size(from_address).await)
     }
 
-    async fn code_hash(&self, from_address: Address, chain_id: u64) -> Result<[u8; 32]> {
+    async fn code_hash(&self, address: Address, chain_id: u64) -> Result<[u8; 32]> {
         // https://eips.ethereum.org/EIPS/eip-1052
         // https://eips.ethereum.org/EIPS/eip-161
 
-        #[maybe_async]
-        async fn data_account_exists<B: AccountStorage>(
-            state: &ExecutorState<'_, B>,
-            address: Address,
-            chain_id: u64,
-        ) -> Result<bool> {
-            Ok(state.nonce(address, chain_id).await? > 0
-                || state.balance(address, chain_id).await? > 0)
+        let code = self.code(address).await?;
+
+        if !code.is_empty() {
+            return Ok(keccak::hash(&code).to_bytes());
         }
 
-        // FIXME: Can we modify self.code to return Option<Buffer> or Option<&[u8]>?
-
-        // FIXME: Because buffer is returned by value, we need to store buffer in order to store a
-        // reference to the bytes. I could use Option<Buffer> instead, but that is storing a more
-        // powerful type than I need. I just need a byte slice.
-        let buffer = self.code(from_address).await?;
-        let bytes_to_hash: Option<&[u8]> = if !buffer.buffer_is_empty() {
-            // A program account exists at the address.
-            Some(&buffer)
-        } else if data_account_exists(self, from_address, chain_id).await? {
-            // A data account exists at the address.
-            Some(&[])
-        } else {
-            // No account exists at the address.
-            None
-        };
+        if self.nonce(address, chain_id).await? == 0 && self.balance(address, chain_id).await? == 0
+        {
+            return Ok(<[u8; 32]>::default());
+        }
 
-        Ok(bytes_to_hash
-            .map(|bytes| solana_program::keccak::hash(bytes).to_bytes())
-            .unwrap_or_default())
+        // equal to "c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"
+        return Ok(keccak::hash(&[]).to_bytes());
     }
 
     async fn code(&self, from_address: Address) -> Result<crate::evm::Buffer> {
@@ -489,3 +473,21 @@ impl<'a, B: AccountStorage> Database for ExecutorState<'a, B> {
         self.backend.contract_chain_id(contract).await
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use hex::FromHex;
+
+    // https://eips.ethereum.org/EIPS/eip-1052
+    #[test]
+    fn test_keccak_hash_empty_slice() {
+        assert_eq!(
+            keccak::hash(&[]).to_bytes(),
+            <[u8; 32]>::from_hex(
+                "c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"
+            )
+            .unwrap()
+        );
+    }
+}