refactor: replace magic value with returnedStatus (#742)

* refactor: replace magic value with returnedStatus

* chore: add suggested changes

---------

Co-authored-by: Jean Cvllr <[email protected]>

constants.ts

@@ -48,7 +48,7 @@ export const INTERFACE_IDS = {
  * Can be used to check if a signature is valid or not.
  */
 export const ERC1271_VALUES = {
-  MAGIC_VALUE: '0x1626ba7e',
+  SUCCESS_VALUE: '0x1626ba7e',
   FAIL_VALUE: '0xffffffff',
 };
 
@@ -59,7 +59,7 @@ export const ERC1271_VALUES = {
  * @dev values returned by the `lsp20VerifyCall` and `lsp20VerifyCallResult` functions of the LSP20 standard.
  * Can be used to check if a calldata payload was check and verified.
  */
-export const LSP20_MAGIC_VALUES = {
+export const LSP20_SUCCESS_VALUES = {
   VERIFY_CALL: {
     // bytes3(keccak256("lsp20VerifyCall(address,address,uint256,bytes)")) + "0x00"
     NO_POST_VERIFICATION: '0x1a238000',

contracts/LSP0ERC725Account/LSP0Constants.sol

@@ -6,7 +6,7 @@ bytes4 constant _INTERFACEID_LSP0 = 0x24871b3d;
 bytes4 constant _INTERFACEID_ERC1271 = 0x1626ba7e;
 
 // ERC1271 - Standard Signature Validation
-bytes4 constant _ERC1271_MAGICVALUE = 0x1626ba7e;
+bytes4 constant _ERC1271_SUCCESSVALUE = 0x1626ba7e;
 bytes4 constant _ERC1271_FAILVALUE = 0xffffffff;
 
 // Ownerhsip Transfer Type IDs

contracts/LSP0ERC725Account/LSP0ERC725AccountCore.sol

@@ -38,7 +38,7 @@ import {
 import {
     _INTERFACEID_LSP0,
     _INTERFACEID_ERC1271,
-    _ERC1271_MAGICVALUE,
+    _ERC1271_SUCCESSVALUE,
     _ERC1271_FAILVALUE,
     _TYPEID_LSP0_OwnershipTransferStarted,
     _TYPEID_LSP0_OwnershipTransferred_SenderNotification,
@@ -205,7 +205,7 @@ abstract contract LSP0ERC725AccountCore is
         }
 
         // If the caller is not the owner, call {lsp20VerifyCall} on the owner
-        // Depending on the magicValue returned, a second call is done after execution
+        // Depending on the returnedStatus, a second call is done after execution
         bool verifyAfter = LSP20CallVerification._verifyCall(accountOwner);
 
         // Perform the execution
@@ -269,7 +269,7 @@ abstract contract LSP0ERC725AccountCore is
         }
 
         // If the caller is not the owner, call {lsp20VerifyCall} on the owner
-        // Depending on the magicValue returned, a second call is done after execution
+        // Depending on the returnedStatus, a second call is done after execution
         bool verifyAfter = LSP20CallVerification._verifyCall(accountOwner);
 
         // Perform the execution
@@ -316,7 +316,7 @@ abstract contract LSP0ERC725AccountCore is
         }
 
         // If the caller is not the owner, call {lsp20VerifyCall} on the owner
-        // Depending on the magicValue returned, a second call is done after setting data
+        // Depending on the returnedStatus, a second call is done after setting data
         bool verifyAfter = _verifyCall(accountOwner);
 
         _setData(dataKey, dataValue);
@@ -369,7 +369,7 @@ abstract contract LSP0ERC725AccountCore is
         }
 
         // If the caller is not the owner, call {lsp20VerifyCall} on the owner
-        // Depending on the magicValue returned, a second call is done after setting data
+        // Depending on the returnedStatus, a second call is done after setting data
         bool verifyAfter = _verifyCall(accountOwner);
 
         for (uint256 i; i < dataKeys.length; ) {
@@ -524,7 +524,7 @@ abstract contract LSP0ERC725AccountCore is
             _inTransferOwnership = false;
         } else {
             // If the caller is not the owner, call {lsp20VerifyCall} on the owner
-            // Depending on the magicValue returned, a second call is done after transferring ownership
+            // Depending on the returnedStatus, a second call is done after transferring ownership
             bool verifyAfter = _verifyCall(currentOwner);
 
             // set the transfer ownership lock
@@ -598,7 +598,7 @@ abstract contract LSP0ERC725AccountCore is
         }
 
         // If the caller is not the owner, call {lsp20VerifyCall} on the owner
-        // Depending on the magicValue returned, a second call is done after transferring ownership
+        // Depending on the returnedStatus, a second call is done after transferring ownership
         bool verifyAfter = _verifyCall(accountOwner);
 
         address previousOwner = owner();
@@ -659,25 +659,25 @@ abstract contract LSP0ERC725AccountCore is
      *
      * 1. If the owner is an EOA, recovers an address from the hash and the signature provided:
      *
-     *      - Returns the `magicValue` if the address recovered is the same as the owner, indicating that it was a valid signature.
+     *      - Returns the `_ERC1271_SUCCESSVALUE` if the address recovered is the same as the owner, indicating that it was a valid signature.
      *
-     *      - If the address is different, it returns the fail value indicating that the signature is not valid.
+     *      - If the address is different, it returns the `_ERC1271_FAILVALUE` indicating that the signature is not valid.
      *
      * 2. If the owner is a smart contract, it forwards the call of {isValidSignature()} to the owner contract:
      *
-     *      - If the contract fails or returns the fail value, the {isValidSignature()} on the account returns the fail value, indicating that the signature is not valid.
+     *      - If the contract fails or returns the `_ERC1271_FAILVALUE`, the {isValidSignature()} on the account returns the `_ERC1271_FAILVALUE`, indicating that the signature is not valid.
      *
-     *      - If the {isValidSignature()} on the owner returned the `magicValue`, the {isValidSignature()} on the account returns the `magicValue`, indicating that it's a valid signature.
+     *      - If the {isValidSignature()} on the owner returned the `_ERC1271_SUCCESSVALUE`, the {isValidSignature()} on the account returns the `_ERC1271_SUCCESSVALUE`, indicating that it's a valid signature.
      *
      * @param dataHash The hash of the data to be validated.
      * @param signature A signature that can validate the previous parameter (Hash).
      *
-     * @return magicValue A `bytes4` value that indicates if the signature is valid or not.
+     * @return returnedStatus A `bytes4` value that indicates if the signature is valid or not.
      */
     function isValidSignature(
         bytes32 dataHash,
         bytes memory signature
-    ) public view virtual override returns (bytes4 magicValue) {
+    ) public view virtual override returns (bytes4 returnedStatus) {
         address _owner = owner();
 
         // If owner is a contract
@@ -692,9 +692,10 @@ abstract contract LSP0ERC725AccountCore is
 
             bool isValid = (success &&
                 result.length == 32 &&
-                abi.decode(result, (bytes32)) == bytes32(_ERC1271_MAGICVALUE));
+                abi.decode(result, (bytes32)) ==
+                bytes32(_ERC1271_SUCCESSVALUE));
 
-            return isValid ? _ERC1271_MAGICVALUE : _ERC1271_FAILVALUE;
+            return isValid ? _ERC1271_SUCCESSVALUE : _ERC1271_FAILVALUE;
         }
         // If owner is an EOA
         else {
@@ -707,11 +708,11 @@ abstract contract LSP0ERC725AccountCore is
                 return _ERC1271_FAILVALUE;
 
             // if recovering is successful and the recovered address matches the owner's address,
-            // return the ERC1271 magic value. Otherwise, return the ERC1271 fail value
+            // return the ERC1271 success value. Otherwise, return the ERC1271 fail value
             // matches the address of the owner, otherwise return fail value
             return
                 recoveredAddress == _owner
-                    ? _ERC1271_MAGICVALUE
+                    ? _ERC1271_SUCCESSVALUE
                     : _ERC1271_FAILVALUE;
         }
     }

contracts/LSP17Extensions/Extension4337.sol

@@ -71,16 +71,16 @@ contract Extension4337 is LSP17Extension, IAccount {
         address owner = LSP14Ownable2Step(msg.sender).owner();
 
         // verify that the recovered address can execute the userOp.callData
-        bytes4 magicValue = ILSP20CallVerifier(owner).lsp20VerifyCall({
+        bytes4 returnedStatus = ILSP20CallVerifier(owner).lsp20VerifyCall({
             callee: msg.sender,
             caller: recovered,
             value: 0,
             receivedCalldata: userOp.callData
         });
 
-        // if the call verifier returns a different magic value, return signature validation failed
+        // if the returnedStatus is a value different than the success value, return signature validation failed
         if (
-            bytes3(magicValue) !=
+            bytes3(returnedStatus) !=
             bytes3(ILSP20CallVerifier.lsp20VerifyCall.selector)
         ) {
             return _SIG_VALIDATION_FAILED;

contracts/LSP20CallVerification/ILSP20CallVerifier.sol

@@ -8,7 +8,7 @@ pragma solidity ^0.8.4;
  */
 interface ILSP20CallVerifier {
     /**
-     * @return magicValue MUST return the first 3 bytes of `lsp20VerifyCall(address,uint256,bytes)` function selector if the call to
+     * @return returnedStatus MUST return the first 3 bytes of `lsp20VerifyCall(address,uint256,bytes)` function selector if the call to
      * the function is allowed, concatened with a byte that determines if the lsp20VerifyCallResult function should
      * be called after the original function call. The byte that invoke the lsp20VerifyCallResult function is strictly `0x01`.
      *
@@ -22,7 +22,7 @@ interface ILSP20CallVerifier {
         address caller,
         uint256 value,
         bytes memory receivedCalldata
-    ) external returns (bytes4 magicValue);
+    ) external returns (bytes4 returnedStatus);
 
     /**
      * @return MUST return the lsp20VerifyCallResult function selector if the call to the function is allowed

contracts/LSP20CallVerification/LSP20CallVerification.sol

@@ -7,7 +7,7 @@ import {ILSP20CallVerifier as ILSP20} from "./ILSP20CallVerifier.sol";
 
 // errors
 import {
-    LSP20InvalidMagicValue,
+    LSP20CallVerificationFailed,
     LSP20CallingVerifierFailed,
     LSP20EOACannotVerifyCall
 } from "./LSP20Errors.sol";
@@ -16,13 +16,13 @@ import {
  * @title Implementation of a contract calling the verification functions according to LSP20 - Call Verification standard.
  *
  * @dev Module to be inherited used to verify the execution of functions according to a verifier address.
- * Verification can happen before or after execution based on a magicValue.
+ * Verification can happen before or after execution based on a returnedStatus.
  */
 abstract contract LSP20CallVerification {
     /**
      * @dev Calls {lsp20VerifyCall} function on the logicVerifier.
-     * Reverts in case the value returned does not match the magic value (lsp20VerifyCall selector)
-     * Returns whether a verification after the execution should happen based on the last byte of the magicValue
+     * Reverts in case the value returned does not match the success value (lsp20VerifyCall selector)
+     * Returns whether a verification after the execution should happen based on the last byte of the returnedStatus
      */
     function _verifyCall(
         address logicVerifier
@@ -42,18 +42,18 @@ abstract contract LSP20CallVerification {
 
         _validateCall(false, success, returnedData);
 
-        bytes4 magicValue = abi.decode(returnedData, (bytes4));
+        bytes4 returnedStatus = abi.decode(returnedData, (bytes4));
 
-        if (bytes3(magicValue) != bytes3(ILSP20.lsp20VerifyCall.selector)) {
-            revert LSP20InvalidMagicValue(false, returnedData);
+        if (bytes3(returnedStatus) != bytes3(ILSP20.lsp20VerifyCall.selector)) {
+            revert LSP20CallVerificationFailed(false, returnedData);
         }
 
-        return magicValue[3] == 0x01;
+        return returnedStatus[3] == 0x01;
     }
 
     /**
      * @dev Calls {lsp20VerifyCallResult} function on the logicVerifier.
-     * Reverts in case the value returned does not match the magic value (lsp20VerifyCallResult selector)
+     * Reverts in case the value returned does not match the success value (lsp20VerifyCallResult selector)
      */
     function _verifyCallResult(
         address logicVerifier,
@@ -79,7 +79,11 @@ abstract contract LSP20CallVerification {
         if (
             abi.decode(returnedData, (bytes4)) !=
             ILSP20.lsp20VerifyCallResult.selector
-        ) revert LSP20InvalidMagicValue(true, returnedData);
+        )
+            revert LSP20CallVerificationFailed({
+                postCall: true,
+                returnedData: returnedData
+            });
     }
 
     function _validateCall(
@@ -94,7 +98,11 @@ abstract contract LSP20CallVerification {
         if (
             returnedData.length < 32 ||
             bytes28(bytes32(returnedData) << 32) != bytes28(0)
-        ) revert LSP20InvalidMagicValue(postCall, returnedData);
+        )
+            revert LSP20CallVerificationFailed({
+                postCall: postCall,
+                returnedData: returnedData
+            });
     }
 
     function _revertWithLSP20DefaultError(

contracts/LSP20CallVerification/LSP20Constants.sol

@@ -8,10 +8,10 @@ bytes4 constant _INTERFACEID_LSP20_CALL_VERIFICATION = 0x1a0eb6a5;
 bytes4 constant _INTERFACEID_LSP20_CALL_VERIFIER = 0xc9dfc532;
 
 // bytes4(bytes.concat(bytes3(ILSP20.lsp20VerifyCall.selector), hex"01"))
-bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION = 0x1a238001;
+bytes4 constant _LSP20_VERIFY_CALL_SUCCESS_VALUE_WITH_POST_VERIFICATION = 0x1a238001;
 
 // bytes4(bytes.concat(bytes3(ILSP20.lsp20VerifyCall.selector), hex"00"))
-bytes4 constant _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION = 0x1a238000;
+bytes4 constant _LSP20_VERIFY_CALL_SUCCESS_VALUE_WITHOUT_POST_VERIFICATION = 0x1a238000;
 
 // bytes4(ILSP20.lsp20VerifyCallResult.selector)
-bytes4 constant _LSP20_VERIFY_CALL_RESULT_MAGIC_VALUE = 0xd3fc45d3;
+bytes4 constant _LSP20_VERIFY_CALL_RESULT_SUCCESS_VALUE = 0xd3fc45d3;

contracts/LSP20CallVerification/LSP20Errors.sol

@@ -8,14 +8,14 @@ pragma solidity ^0.8.4;
 error LSP20CallingVerifierFailed(bool postCall);
 
 /**
- * @dev reverts when the call to the owner does not return the magic value
+ * @dev reverts when the call to the owner does not return the LSP20 success value
  * @param postCall True if the execution call was done, False otherwise
  * @param returnedData The data returned by the call to the logic verifier
  */
-error LSP20InvalidMagicValue(bool postCall, bytes returnedData);
+error LSP20CallVerificationFailed(bool postCall, bytes returnedData);
 
 /**
- * @dev Reverts when the logic verifier is an Externally Owned Account (EOA) that cannot return the LSP20 magic value.
+ * @dev Reverts when the logic verifier is an Externally Owned Account (EOA) that cannot return the LSP20 success value.
  * @param logicVerifier The address of the logic verifier
  */
 error LSP20EOACannotVerifyCall(address logicVerifier);

contracts/LSP6KeyManager/LSP6KeyManagerCore.sol

@@ -51,7 +51,7 @@ import {
 
 import {
     _INTERFACEID_ERC1271,
-    _ERC1271_MAGICVALUE,
+    _ERC1271_SUCCESSVALUE,
     _ERC1271_FAILVALUE
 } from "../LSP0ERC725Account/LSP0Constants.sol";
 import {
@@ -61,9 +61,9 @@ import {
 } from "./LSP6Constants.sol";
 import {
     _INTERFACEID_LSP20_CALL_VERIFIER,
-    _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION,
-    _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION,
-    _LSP20_VERIFY_CALL_RESULT_MAGIC_VALUE
+    _LSP20_VERIFY_CALL_SUCCESS_VALUE_WITHOUT_POST_VERIFICATION,
+    _LSP20_VERIFY_CALL_SUCCESS_VALUE_WITH_POST_VERIFICATION,
+    _LSP20_VERIFY_CALL_RESULT_SUCCESS_VALUE
 } from "../LSP20CallVerification/LSP20Constants.sol";
 import {_INTERFACEID_LSP25} from "../LSP25ExecuteRelayCall/LSP25Constants.sol";
 
@@ -143,14 +143,14 @@ abstract contract LSP6KeyManagerCore is
      * @inheritdoc IERC1271
      *
      * @dev Checks if a signature was signed by a controller that has the permission `SIGN`.
-     * If the signer is a controller with the permission `SIGN`, it will return the ERC1271 magic value.
+     * If the signer is a controller with the permission `SIGN`, it will return the ERC1271 success value.
      *
-     * @return magicValue `0x1626ba7e` on success, or `0xffffffff` on failure.
+     * @return returnedStatus `0x1626ba7e` on success, or `0xffffffff` on failure.
      */
     function isValidSignature(
         bytes32 dataHash,
         bytes memory signature
-    ) public view virtual override returns (bytes4 magicValue) {
+    ) public view virtual override returns (bytes4 returnedStatus) {
         // if isValidSignature fail, the error is catched in returnedError
         (address recoveredAddress, ECDSA.RecoverError returnedError) = ECDSA
             .tryRecover(dataHash, signature);
@@ -159,12 +159,12 @@ abstract contract LSP6KeyManagerCore is
         if (returnedError != ECDSA.RecoverError.NoError)
             return _ERC1271_FAILVALUE;
 
-        // if the address recovered has SIGN permission return the ERC1271 magic value, otherwise the fail value
+        // if the address recovered has SIGN permission return the ERC1271 success value, otherwise the fail value
         return (
             ERC725Y(_target).getPermissionsFor(recoveredAddress).hasPermission(
                 _PERMISSION_SIGN
             )
-                ? _ERC1271_MAGICVALUE
+                ? _ERC1271_SUCCESSVALUE
                 : _ERC1271_FAILVALUE
         );
     }
@@ -313,9 +313,9 @@ abstract contract LSP6KeyManagerCore is
      * Anyone can call this function to verify if the `caller` has the right permissions to perform the abi-encoded function call `data`
      * on the {`target`} contract (while sending `msgValue` alongside the call).
      *
-     * If the permissions have been verified successfully and `caller` is authorized, one of the following two LSP20 magic value will be returned:
-     *  - `0x1a238000`: LSP20 magic value **without** post verification (last byte is `0x00`).
-     *  - `0x1a238001`: LSP20 magic value **with** post-verification (last byte is `0x01`).
+     * If the permissions have been verified successfully and `caller` is authorized, one of the following two LSP20 success value will be returned:
+     *  - `0x1a238000`: LSP20 success value **without** post verification (last byte is `0x00`).
+     *  - `0x1a238001`: LSP20 success value **with** post-verification (last byte is `0x01`).
      */
     function lsp20VerifyCall(
         address targetContract,
@@ -341,8 +341,8 @@ abstract contract LSP6KeyManagerCore is
             // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function
             return
                 isSetData || reentrancyStatus
-                    ? _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION
-                    : _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION;
+                    ? _LSP20_VERIFY_CALL_SUCCESS_VALUE_WITHOUT_POST_VERIFICATION
+                    : _LSP20_VERIFY_CALL_SUCCESS_VALUE_WITH_POST_VERIFICATION;
         }
         /// @dev If a different address is invoking the verification,
         /// do not change the state or emit the event to allow read-only verification
@@ -362,8 +362,8 @@ abstract contract LSP6KeyManagerCore is
             // if it's a setData call, do not invoke the `lsp20VerifyCallResult(..)` function
             return
                 isSetData || reentrancyStatus
-                    ? _LSP20_VERIFY_CALL_MAGIC_VALUE_WITHOUT_POST_VERIFICATION
-                    : _LSP20_VERIFY_CALL_MAGIC_VALUE_WITH_POST_VERIFICATION;
+                    ? _LSP20_VERIFY_CALL_SUCCESS_VALUE_WITHOUT_POST_VERIFICATION
+                    : _LSP20_VERIFY_CALL_SUCCESS_VALUE_WITH_POST_VERIFICATION;
         }
     }
 
@@ -375,11 +375,11 @@ abstract contract LSP6KeyManagerCore is
         bytes memory /*result*/
     ) external virtual override returns (bytes4) {
         // If it's the target calling, set back the reentrancy guard
-        // to false, if not return the magic value
+        // to false, if not return the success value
         if (msg.sender == _target) {
             _nonReentrantAfter(msg.sender);
         }
-        return _LSP20_VERIFY_CALL_RESULT_MAGIC_VALUE;
+        return _LSP20_VERIFY_CALL_RESULT_SUCCESS_VALUE;
     }
 
     function _execute(