From 43b75c7953911e96aa73be7a1a4f4eea50754988 Mon Sep 17 00:00:00 2001
From: Greg Hysz <hysz@users.noreply.github.com>
Date: Mon, 30 Nov 2020 09:50:20 -0800
Subject: [PATCH] minor doc updates: include Best Practices for Feature
 Development. (#58)

---
 docs/advanced/mtx.rst          |  3 ++-
 docs/advanced/uniswap.rst      |  2 ++
 docs/architecture/features.rst | 27 ++++++++++++++++++++++++++-
 docs/architecture/proxy.rst    | 10 +++++++++-
 4 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/docs/advanced/mtx.rst b/docs/advanced/mtx.rst
index ce3be54779..9844ecc671 100644
--- a/docs/advanced/mtx.rst
+++ b/docs/advanced/mtx.rst
@@ -100,6 +100,7 @@ This call will revert in the following scenarios:
 - The mtx has expired.
 - The Ethereum transaction's gas price (``tx.gasprice``) is outside of the range ``[mtx.minGasPrice..mtx.maxGasPrice]``
 - The ETH sent with the mtx is less than ``mtx.value``
+- The allowance/balance of ``signer`` is insufficient to pay ``feeAmount`` of ``feeToken`` to the ``sender`` (if specified)
 - The signature is invalid.
 - The mtx was already executed
 - The underlying function is not supported by meta-transactions (see list above).
@@ -122,7 +123,7 @@ batchExecuteMetaTransactions
         payable
         returns (bytes[] memory returnResults);
 
-A `MetaTransactionExecuted <../basics/events.html#metatransactionexecuted>`_ event is emitted for each mtx on succes. The ``returnResult`` contains the raw return data for the executed function This call will revert if the one of the ``mtxs`` reverts.
+A `MetaTransactionExecuted <../basics/events.html#metatransactionexecuted>`_ event is emitted for each mtx on succes. The ``returnResult`` contains the raw return data for the executed function This call will revert if the one of the ``mtxs`` reverts. Any exceess Ether will be refunded to the ``msg.sender``.
 
 
 getMetaTransactionExecutedBlock
diff --git a/docs/advanced/uniswap.rst b/docs/advanced/uniswap.rst
index 6f8c82b425..aa11d536e2 100644
--- a/docs/advanced/uniswap.rst
+++ b/docs/advanced/uniswap.rst
@@ -24,6 +24,8 @@ The 0x Protocol is equipped with a highly optimized `UniswapV2 Router <https://u
 
 This function sells ``sellAmount`` of ``tokens[0]`` for at least ``minBuyAmount`` of ``tokens[-1]``. The ``tokens`` array defines how to route the trade between Uniswap pools. This function does not emit any events, although Uniswap pools will emit their own events. This function reverts if amount bought from Uniswap is less than ``minBuyAmount``, or if Uniswap reverts.
 
+See the source code for our router `here <https://github.com/0xProject/protocol/blob/development/contracts/zero-ex/contracts/src/features/UniswapFeature.sol>`_.
+
 See the official `Uniswap V2 Documentation <https://uniswap.org/docs/v2/>`_ for information on events/reverts/allowances.
 
 .. note::
diff --git a/docs/architecture/features.rst b/docs/architecture/features.rst
index c8379d86bf..c40997e720 100644
--- a/docs/architecture/features.rst
+++ b/docs/architecture/features.rst
@@ -49,4 +49,29 @@ The only requirement is that the Feature implements the interface in `IFeature <
 
         /// @dev The version of this feature set.
         function FEATURE_VERSION() external view returns (uint256 version);
-    }
\ No newline at end of file
+    }
+
+
+Best Practices
+================
+
+We use this checklist to review the safety of new Features.
+
+.. code-block::
+
+    - [ ] Feature has updated version information.
+    - [ ] implements IFeature interface.
+    - [ ] Feature contracts are stateless (including inherited contracts).
+    - [ ] onlySelf feature functions are prefixed with _.
+    - [ ] Feature functions are added to full_migration_tests.
+    - [ ] No delegatecalls from inside a feature. Call other features through the router.
+    - [ ] No self-destruct in features (except BootstrapFeature).
+    - [ ] No intentionally persistent (non-atomic) balances on the Exchange Proxy.
+    - [ ] No direct access to another feature’s storage bucket without strong justification.
+    - [ ] No executing arbitrary calldata from the context of the Exchange Proxy.
+    - [ ] No external calls to arbitrary contracts from within the Exchange Proxy.
+    - [ ] Features use unique StorageIds. 
+    - [ ] Document functions with execution contexts outside of the Exchange Proxy.
+    - [ ] Document feature dependencies in checklist doc.
+    - [ ] Document reentrant functions in checklist doc.
+    - [ ] Document temporary balances.
diff --git a/docs/architecture/proxy.rst b/docs/architecture/proxy.rst
index 0d9971b06e..a6369e3793 100644
--- a/docs/architecture/proxy.rst
+++ b/docs/architecture/proxy.rst
@@ -246,4 +246,12 @@ Although the proxy will not have access to the V3 asset proxies initially, early
 
 **Balances**
 
-Inevitably, there will be features that will cause the Exchange Proxy to hold temporary balances (e.g., payable functions). Thus, it’s a good idea that no feature should cause the Exchange Proxy to hold a permanent balance of tokens or ether, since these balances can easily get mixed up with temporary balances.
\ No newline at end of file
+Inevitably, there will be features that will cause the Exchange Proxy to hold temporary balances (e.g., payable functions). Thus, it’s a good idea that no feature should cause the Exchange Proxy to hold a permanent balance of tokens or ether, since these balances can easily get mixed up with temporary balances.
+
+**Reentrancy**
+
+Functions can be re-entered by default; those secured by the ``nonReentrant`` modifier cannot be re-entered.
+
+**Colliding Function Selectors**
+
+We manually ensure that function selectors do not collide during PR's. See the `Feature Checklist <./features.html#best-practices>`_ for a complete list of our best practices on Feature Development.
\ No newline at end of file