Merge pull request #211 from CMTA/coherence-name

Coherence name with OZ
CMTA · Aug 7, 2023 · a4c660f · a4c660f
2 parents 7faeda7 + fe73847
commit a4c660f
Show file tree

Hide file tree

Showing 14 changed files with 341 additions and 256 deletions.
diff --git a/FAQ.md b/FAQ.md
@@ -1,81 +1,105 @@
 # FAQ
 
-## IDE (Truffle, Hardhat, ...)
+This FAQ is intended to developers familiar with smart contracts
+development.
+
+## Toolkit support
 
 > Why do you continue using Truffle instead of migrating to HardHat or Foundry?
 
-**Hardhat VS Truffle**
+Regarding [Hardhat](https://hardhat.org/):
 
 - Our tests are not working with Hardhat so to migrate to hardhat, we will have to update our tests which will require a lot of works.
 - Moreover, we do not see a use case where hardhat will be better than Truffle.
 - Hardhat has a lot of plugins, but for example, for the coverage, we can run the coverage without be fully compatible with Hardhat.
 
-**Truffle VS  Foundry**
+Regarding [Foundry](https://book.getfoundry.sh/):
 
 -  The plugin "upgrades plugin" by OpenZeppelin is not available with Foundry and it is a very good tool to check the proxy implementation and perform automatic tests. See [https://docs.openzeppelin.com/upgrades-plugins/1.x/](https://docs.openzeppelin.com/upgrades-plugins/1.x/)
--  The tests for the gasless module (MetaTx) will be difficult to write in Solidity, see [https://github.com/CMTA/CMTAT/blob/master/test/common/MetaTxModuleCommon.js](https://github.com/CMTA/CMTAT/blob/master/test/common/MetaTxModuleCommon.js)
-- OpenZeppelin, the main libraries we use, have their tests mainly written in JavaScript, so it provides good examples for our tests
-- But for performance, we have seen indeed that Foundry is better than Truffle, notably to test the Snapshot Module
+-  The tests for the gasless module (MetaTx) would be difficult to write
+   in Solidity, as Foundry requires, see [https://github.com/CMTA/CMTAT/blob/master/test/common/MetaTxModuleCommon.js](https://github.com/CMTA/CMTAT/blob/master/test/common/MetaTxModuleCommon.js)
+- The OpenZeppelin libraries that we use have their tests mainly written in JavaScript, which provides a good basis for our tests
+- Performance wise, we observed that Foundry is superior to Truffle, notably to test the Snapshot module
+- We have a repository [CMTA/CMTAT-Foundry](https://github.com/CMTA/CMTAT-foundry) that provides experimental support for Foundry, but it does not provide complete support and testing for the latest CMTAT version.
+
 
->  Do you plan to support Foundry in the near Future? I see a CMTAT-Foundry repo. Is it reliable?
+>  Do you plan to fully support Foundry in the near future? 
 
-No, it is currently not reliable.
+For the foreseeable future, we plan to keep Truffle as the main
+development and testing suite.
 
-We have not planned to export all the tests in their Solidity version, but some tests are available
+We have not planned to export all the tests from the Truffle suite to
+their Solidity version equivalent suitable to Foundry, though some tests
+are already available.
 
-The repo CMTAT-Foundry will have the latest CMTAT version
+The CMTAT-Foundry repository uses CMTAT as a submodule, whose version is
+documented in its
+[README](https://github.com/CMTA/CMTAT-Foundry/blob/main/README.md#cmtat---using-the-foundry-suite).
 
-Please, note that we provide only a minimal support for the foundry repository as well as Hardhat.
 
-We use Truffle to maintain the project.
+>  Can Hardhat be used to run tests?
 
->  Hardhat tests: are they really working in v2.3.0?
+No, please use Truffle to run the tests.
 
-No, please use Truffle to run the tests
 
 ## Modules
 
->  Why the Snapshot module is not audited in the version v2.3.0?
+> What is the reason the Snapshot module wasn't audited in version v2.3.0?
 
-It was out of scope because it’s not really used yet and will likely be subject to changes soon.
+This module was left out of scope because it is not used yet (and not
+included in a default deployment) and will be
+subject to changes soon. 
 
-At deployment, this module is not included by default
+> What is the status of [ERC1404](https://erc1404) compatibility?
 
-> What is the status for ERC1404 compatibility?
+We have not planned to be fully compatible with ERC1404 (which, in fact,
+is only an EIP at the time of writing). 
+CMTAT includes the two functions defind by ERC1404, namely
+`detectTransferRestriction` and `messageForTransferRestriction`.
+Thus CMTAT can provide the same functionality as ERC1404.
 
-We have not planned to be fully compatible since this ERC is not an ERC, it is only an EIP. 
+However, from a pure technical perspective, CMTAT is not fully compliant
+with the ERC1404 specification, due the way it inherits the ERC20
+interface. 
 
-To be fully compatible, we have to inherit of ERC20 inside the interface and it will break our architecture.
+> What is the purpose of the flag parameter in the Base module?
 
-See [https://erc1404.org/](https://erc1404.org/)
+It is just a variable to include some additional information under the form of bit fields.
+It is not used inside the code because it is destined to provide more
+information on the tokens to the "outside", for example for the token
+owners.
 
-> What is exactly the purpose of the flag parameter in BaseModule?
-> I see that it’s a variable (uint256) to include some information, but I don’t see any use case in the code.
 
-It is just a variable to include some additional information under the form of bit flags.
-It is not used inside the code because it is destined to provide more information on the tokens to the "outside", for example for the token owners.
+> Is the Validation module optional? 
 
+Generally, for a CMTAT token, the Validation functionality is optional
+from the legal perspective (please contact [email protected] for detailed
+information).
 
+However, in order to use the functions from the Pause and Enforcement
+modules, our CMTAT implementation requires the Validation module
+Therefore, the Validation module is effectively required *in this
+implementation*. 
 
->  Question regarding the ValidationModule optional module. 
->
-> Why is it optional? The module is required by Pauser and Enforcer mandatory modules
+If you remove the Validation module and want to use the Pause or the
+Enforcement module, you have to call the functions of modules inside the
+main contracts. It was initially the case but we have changed this
+behaviour when addressing an issue reported by a security audit.
+Here is an old version:
+[https://github.com/CMTA/CMTAT/blob/ed23bfc69cfacc932945da751485c6472705c975/contracts/CMTAT.sol#L205](https://github.com/CMTA/CMTAT/blob/ed23bfc69cfacc932945da751485c6472705c975/contracts/CMTAT.sol#L205),
+and the relevant Pull [Request](https://github.com/CMTA/CMTAT/pull/153).
 
-- ValidationModule is optional from the legal perspective, but you can ask [email protected] to have a better/clearer information on that.
-- It is the opposite: PauseModule and EnforcementModule are required to use the ValidationModule (but indeed, you actually need the ValidationModule for the functions to be called)
-- If you remove the ValidationModule and want to use the Pause and Enforcement module, you have to call the functions of modules inside the main contracts. It was initially the case but we have changed this behaviour by fixing the CVF-1
-Here an old version: [https://github.com/CMTA/CMTAT/blob/ed23bfc69cfacc932945da751485c6472705c975/contracts/CMTAT.sol#L205](https://github.com/CMTA/CMTAT/blob/ed23bfc69cfacc932945da751485c6472705c975/contracts/CMTAT.sol#L205)
-The PR: [https://github.com/CMTA/CMTAT/pull/153](https://github.com/CMTA/CMTAT/pull/153)
-We could probably move the ValidationModule inside the mandatory modules and think about a better architecture (but probably not for the next release)
 
 ## Documentation
 
-> What is the code coverage?
+> What is the code coverage of the test suite?
 
-A code coverage is available here: [https://github.com/CMTA/CMTAT/blob/master/doc/general/test/coverage/index.html](https://github.com/CMTA/CMTAT/blob/master/doc/general/test/coverage/index.html)
+A [code coverage report](https://github.com/CMTA/CMTAT/blob/master/doc/general/test/coverage/index.html)
+is available.
 
-Normally, you can run the code coverage with `npx hardhat coverage`
+Normally, you can run the test suite and generate a code coverage report with `npx hardhat coverage`.
 
-Please clone the repository and open the file inside your navigator
+Please clone the repository and open the file inside your browser.
 
-You will find a summary of all automatic tests in the file [test.pdf](https://github.com/CMTA/CMTAT/blob/master/doc/general/test/test.pdf)
+You will find a summary of all automatic tests in
+[test.pdf](https://github.com/CMTA/CMTAT/blob/master/doc/general/test/test.pdf).
diff --git a/contracts/modules/wrapper/mandatory/BurnModule.sol b/contracts/modules/wrapper/mandatory/BurnModule.sol
@@ -7,7 +7,10 @@ import "../../../../openzeppelin-contracts-upgradeable/contracts/proxy/utils/Ini
 import "../../security/AuthorizationModule.sol";
 
 abstract contract BurnModule is ERC20Upgradeable, AuthorizationModule {
-    event Burn(address indexed owner, uint256 amount, string reason);
+    /**
+    * @notice Emitted when the specified `value` amount of tokens owned by `owner`are destroyed with the given `reason`
+    */
+    event Burn(address indexed owner, uint256 value, string reason);
 
     function __BurnModule_init(
         string memory name_,
@@ -35,51 +38,56 @@ abstract contract BurnModule is ERC20Upgradeable, AuthorizationModule {
     }
 
     /**
-     * @dev Destroys `amount` tokens from `account`
-     *
+     * @notice Destroys a `value` amount of tokens from `account`, by transferring it to address(0).
+     * @dev  
      * See {ERC20-_burn}
      * Emits a {Burn} event
+     * Emits a {Transfer} event with `to` set to the zero address  (emits inside _burn).
+     * Requirements:
+     * - the caller must have the `BURNER_ROLE`.
      */
     function forceBurn(
         address account,
-        uint256 amount,
+        uint256 value,
         string memory reason
     ) public onlyRole(BURNER_ROLE) {
-        _burn(account, amount);
-        emit Burn(account, amount, reason);
+        _burn(account, value);
+        emit Burn(account, value, reason);
     }
 
     /**
      *
-     * @dev batch version of {forceBurn}.
-     *
+     * @notice batch version of {forceBurn}.
+     * @dev 
      * See {ERC20-_burn} and {OpenZeppelin ERC1155_burnBatch}.
      *
-     * Emits a {Burn} event by burn action.
-     *
+     * For each burn action:
+     * -Emits a {Burn} event
+     * -Emits a {Transfer} event with `to` set to the zero address  (emits inside _burn).
+     * The burn `reason`is the same for all `accounts` which tokens are burnt.
      * Requirements:
-     * - `tos` and `amounts` must have the same length
+     * - `accounts` and `values` must have the same length
      * - the caller must have the `BURNER_ROLE`.
      */
     function forceBurnBatch(
         address[] calldata accounts,
-        uint256[] calldata amounts,
+        uint256[] calldata values,
         string memory reason
     ) public onlyRole(BURNER_ROLE) {
         require(
             accounts.length > 0,
-            "CMTAT: tos is empty"
+            "CMTAT: accounts is empty"
         );
-        // We do not check that amounts is not empty since
+        // We do not check that values is not empty since
         // this require will throw an error in this case.
         require(
-            accounts.length == amounts.length,
-            "CMTAT: accounts and amounts length mismatch"
+            accounts.length == values.length,
+            "CMTAT: accounts and values length mismatch"
         );
 
         for (uint256 i = 0; i < accounts.length; ) {
-            _burn(accounts[i], amounts[i]);
-            emit Burn(accounts[i], amounts[i], reason);
+            _burn(accounts[i], values[i]);
+            emit Burn(accounts[i], values[i], reason);
             unchecked {
                 ++i;
             }