EVM: address some TODOs (#1177)

Fixes filecoin-project/ref-fvm#1407
Fixes #1084

actors/evm/shared/src/address.rs

@@ -5,8 +5,6 @@ use fvm_shared::address::Address;
 use fvm_shared::ActorID;
 
 /// A Filecoin address as represented in the FEVM runtime (also called EVM-form).
-///
-/// TODO this type will eventually handle f4 address detection.
 #[derive(serde::Deserialize, serde::Serialize, PartialEq, Eq, Clone, Copy)]
 pub struct EthAddress(#[serde(with = "strict_bytes")] pub [u8; 20]);
 

actors/evm/src/interpreter/instructions/context.rs

@@ -19,10 +19,9 @@ pub fn blockhash(
         .try_into()
         .ok()
         .filter(|&height: &ChainEpoch| {
-            // The EVM allows fetching blockhashes from the 256 _previous_ blocks.
-            // TODO: we can consider extending this to allow the full range.
-            // Also relates to https://github.com/filecoin-project/ref-fvm/issues/1023 (we might
-            // want to keep some of these restrictions).
+            // The EVM allows fetching blockhashes from the 256 _previous_ blocks, not including the
+            // current. The FVM allows fetching block CIDs from the last 899 epochs, not including
+            // the current epoch.
             let curr_epoch = system.rt.curr_epoch();
             height >= curr_epoch - 256 && height < curr_epoch
         })

actors/evm/src/interpreter/instructions/lifecycle.rs

@@ -45,6 +45,7 @@ pub fn create(
         &[]
     };
 
+    // We increment the nonce earlier than in the EVM. See the comment in `create2` for details.
     let nonce = system.increment_nonce();
     let params = eam::CreateParams { code: input_data.to_vec(), nonce };
     create_init(system, IpldBlock::serialize_cbor(&params)?, eam::CREATE_METHOD_NUM, value)
@@ -64,7 +65,6 @@ pub fn create2(
 
     let ExecutionState { stack: _, memory, .. } = state;
 
-    // see `create()` overall TODOs
     let endowment = TokenAmount::from(&endowment);
     if endowment > system.rt.current_balance() {
         return Ok(U256::zero());
@@ -82,6 +82,16 @@ pub fn create2(
     };
     let params = eam::Create2Params { code: input_data.to_vec(), salt };
 
+    // We increment the nonce earlier than in the EVM, but this is unlikely to cause issues:
+    //
+    // 1. Like the EVM, we increment the nonce on address conflict (effectively "skipping" the
+    //    address).
+    // 2. Like the EVM, we increment the nonce even if the target contract runs out of gas.
+    // 4. Like the EVM, we don't increment the nonce if the caller doesn't have enough funds to
+    //    cover the endowment.
+    // 4. Unlike the EVM, we increment the nonce if contract creation fails because we're at the max
+    //    stack depth. However, given that there are other ways to increment the nonce without
+    //    deploying a contract (e.g., 2), this shouldn't be an issue.
     system.increment_nonce();
     create_init(system, IpldBlock::serialize_cbor(&params)?, eam::CREATE2_METHOD_NUM, endowment)
 }
@@ -101,25 +111,6 @@ fn create_init(
     let ret =
         system.send(&EAM_ACTOR_ADDR, method, params, value, Some(gas_limit), SendFlags::default());
 
-    // https://github.com/ethereum/go-ethereum/blob/fb75f11e87420ec25ff72f7eeeb741fa8974e87e/core/vm/evm.go#L406-L496
-    // Normally EVM will do some checks here to ensure that a contract has the capability
-    // to create an actor, but here FVM does these checks for us, including:
-    // - execution depth, equal to FVM's max call depth (FVM)
-    // - account has enough value to send (FVM)
-    // - ensuring there isn't an existing account at the generated f4 address (INIT)
-    // - constructing smart contract on chain (INIT)
-    // - checks if max code size is exceeded (EAM & EVM)
-    // - gas cost of deployment (FVM)
-    // - EIP-3541 (EVM)
-    //
-    // However these errors are flattened to a 0 pushed on the stack.
-
-    // TODO revert state if error was returned (revert nonce bump)
-    // https://github.com/filecoin-project/ref-fvm/issues/956
-
-    // TODO Exit with revert if sys out of gas when subcall gas limits are introduced
-    // https://github.com/filecoin-project/ref-fvm/issues/966
-
     Ok(match ret {
         Ok(eam_ret) => {
             let ret: eam::CreateReturn = deserialize_block(eam_ret)?;

actors/evm/src/interpreter/precompiles/fvm.rs

@@ -166,7 +166,7 @@ pub(super) fn call_actor_shared<RT: Runtime>(
     // ------ Begin Call -------
 
     let result = {
-        // TODO only CBOR or "nothing" for now
+        // TODO only CBOR or "nothing" for now. We should support RAW and DAG_CBOR in the future.
         let params = match codec {
             fvm_ipld_encoding::CBOR => Some(IpldBlock { codec, data: params.into() }),
             #[cfg(feature = "hyperspace")]

actors/evm/tests/precompile.rs

@@ -282,8 +282,6 @@ fn test_precompile_transfer_nothing() {
 
 #[test]
 fn test_precompile_failure() {
-    // TODO: refactor these to be more clear
-
     let bytecode = resolve_address_contract();
     let mut rt = util::construct_and_verify(bytecode);