feat(cryptography): drop the TIP712 storage fallback for long name/version

.changeset/tip712-drop-storage-fallback.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-tron-solidity': minor
+---
+
+`TIP712`: Drop the storage fallback for long `name`/`version` values. Both parameters must now fit in a `ShortString` (at most 31 bytes) or the constructor reverts with `ShortStrings.StringTooLong`. Storing the domain exclusively in immutables keeps the domain (and downstream `TRC7739` verification) consistent when the contract is used behind a proxy or clone without an initializer.

contracts/utils/cryptography/TIP712.sol

@@ -36,6 +36,10 @@ import {ITRC5267} from "../../interfaces/ITRC5267.sol";
  * separator of the implementation contract. This will cause the {_domainSeparatorV4} function to always rebuild the
  * separator from the immutable values, which is cheaper than accessing a cached version in cold storage.
  *
+ * IMPORTANT: The `name` and `version` must each fit in a `ShortString` (at most 31 bytes). Longer values cause the
+ * constructor to revert with a `ShortStrings.StringTooLong` error. Because the values are stored exclusively in
+ * immutables, the domain is preserved when the contract is used behind a proxy or clone without an initializer.
+ *
  * @custom:oz-upgrades-unsafe-allow state-variable-immutable
  */
 abstract contract TIP712 is ITRC5267 {
@@ -55,8 +59,14 @@ abstract contract TIP712 is ITRC5267 {
 
     ShortString private immutable _name;
     ShortString private immutable _version;
+
+    // IMPORTANT: Deprecated. Kept to preserve the storage layout of inheriting contracts used as an
+    // implementation behind a proxy.
     // slither-disable-next-line constable-states
     string private _nameFallback;
+
+    // IMPORTANT: Deprecated. Kept to preserve the storage layout of inheriting contracts used as an
+    // implementation behind a proxy.
     // slither-disable-next-line constable-states
     string private _versionFallback;
 
@@ -71,20 +81,10 @@ abstract contract TIP712 is ITRC5267 {
      *
      * NOTE: These parameters cannot be changed except through a xref:learn::upgrading-smart-contracts.adoc[smart
      * contract upgrade].
-     *
-     * WARNING: This concerns the constructor-based variant of this contract. When `name` or `version` does not fit in
-     * a `ShortString` (i.e. is 32 bytes or longer), the constructor writes it to the `_nameFallback`/`_versionFallback`
-     * storage variables. Under a `delegatecall`-based deployment (minimal proxy/clone) that constructor never runs in
-     * the proxy's storage context, so the fallbacks stay empty while {_domainSeparatorV4} still uses the
-     * implementation's immutable `_hashedName`/`_hashedVersion`. As a result {eip712Domain} reports an empty
-     * `name`/`version` that does not match the separator used for verification, breaking off-chain domain discovery.
-     * Keep `name` and `version` within 31 bytes in that case. The upgradeable variant is not affected: it stores
-     * `name` and `version` as plain strings in namespaced storage, written by its initializer and read back by both
-     * {_domainSeparatorV4} and {eip712Domain}.
      */
     constructor(string memory name, string memory version) {
-        _name = name.toShortStringWithFallback(_nameFallback);
-        _version = version.toShortStringWithFallback(_versionFallback);
+        _name = name.toShortString();
+        _version = version.toShortString();
         _hashedName = keccak256(bytes(name));
         _hashedVersion = keccak256(bytes(version));
 
@@ -159,22 +159,20 @@ abstract contract TIP712 is ITRC5267 {
     /**
      * @dev The name parameter for the TIP712 domain.
      *
-     * NOTE: By default this function reads _name which is an immutable value.
-     * It only reads from storage if necessary (in case the value is too large to fit in a ShortString).
+     * NOTE: This function reads `_name`, which is an immutable value.
      */
     // solhint-disable-next-line func-name-mixedcase
     function _TIP712Name() internal view returns (string memory) {
-        return _name.toStringWithFallback(_nameFallback);
+        return _name.toString();
     }
 
     /**
      * @dev The version parameter for the TIP712 domain.
      *
-     * NOTE: By default this function reads _version which is an immutable value.
-     * It only reads from storage if necessary (in case the value is too large to fit in a ShortString).
+     * NOTE: This function reads `_version`, which is an immutable value.
      */
     // solhint-disable-next-line func-name-mixedcase
     function _TIP712Version() internal view returns (string memory) {
-        return _version.toStringWithFallback(_versionFallback);
+        return _version.toString();
     }
 }

scripts/upgradeable/upgradeable.patch

@@ -1,13 +1,13 @@
 diff --git a/contracts/package.json b/contracts/package.json
-index 9432b1b..ea1db09 100644
+index bdacfb6..c12f2ea 100644
 --- a/contracts/package.json
 +++ b/contracts/package.json
 @@ -1,5 +1,5 @@
  {
 -  "name": "@openzeppelin/tron-contracts",
 +  "name": "@openzeppelin/tron-contracts-upgradeable",
    "description": "Secure Smart Contract library for Solidity for Tron",
-   "version": "0.0.1",
+   "version": "5.6.0",
    "files": [
 @@ -13,7 +13,7 @@
    },
@@ -18,7 +18,7 @@ index 9432b1b..ea1db09 100644
    },
    "keywords": [
      "solidity",
-@@ -27,5 +27,8 @@
+@@ -30,5 +30,8 @@
    "license": "MIT",
    "bugs": {
      "url": "https://github.com/OpenZeppelin/tron-contracts/issues"
@@ -28,7 +28,7 @@ index 9432b1b..ea1db09 100644
    }
  }
 diff --git a/contracts/utils/ReentrancyGuard.sol b/contracts/utils/ReentrancyGuard.sol
-index c156fa1..895e393 100644
+index 3d8bc81..4dae6f2 100644
 --- a/contracts/utils/ReentrancyGuard.sol
 +++ b/contracts/utils/ReentrancyGuard.sol
 @@ -36,6 +36,11 @@ abstract contract ReentrancyGuard {
@@ -55,7 +55,7 @@ index c156fa1..895e393 100644
       * @dev Prevents a contract from calling itself, directly or indirectly.
       * Calling a `nonReentrant` function from another `nonReentrant`
 diff --git a/contracts/utils/cryptography/TIP712.sol b/contracts/utils/cryptography/TIP712.sol
-index 5446d00..13b4175 100644
+index 89b0afa..f26da77 100644
 --- a/contracts/utils/cryptography/TIP712.sol
 +++ b/contracts/utils/cryptography/TIP712.sol
 @@ -4,7 +4,6 @@
@@ -66,14 +66,18 @@ index 5446d00..13b4175 100644
  import {ITRC5267} from "../../interfaces/ITRC5267.sol";
 
  /**
-@@ -32,33 +31,15 @@ import {ITRC5267} from "../../interfaces/ITRC5267.sol";
+@@ -32,43 +31,15 @@ import {ITRC5267} from "../../interfaces/ITRC5267.sol";
   * NOTE: This contract implements the version of the encoding known as "v4", as implemented by the JSON RPC method
   * https://docs.metamask.io/guide/signing-data.html[`eth_signTypedDataV4` in MetaMask].
   *
 - * NOTE: In the upgradeable version of this contract, the cached values will correspond to the address, and the domain
 - * separator of the implementation contract. This will cause the {_domainSeparatorV4} function to always rebuild the
 - * separator from the immutable values, which is cheaper than accessing a cached version in cold storage.
 - *
+- * IMPORTANT: The `name` and `version` must each fit in a `ShortString` (at most 31 bytes). Longer values cause the
+- * constructor to revert with a `ShortStrings.StringTooLong` error. Because the values are stored exclusively in
+- * immutables, the domain is preserved when the contract is used behind a proxy or clone without an initializer.
+- *
 - * @custom:oz-upgrades-unsafe-allow state-variable-immutable
 + * NOTE: The upgradeable version of this contract does not use an immutable cache and recomputes the domain separator
 + * each time {_domainSeparatorV4} is called. This is cheaper than accessing a cached version in cold storage.
@@ -95,21 +99,27 @@ index 5446d00..13b4175 100644
 -
 -    ShortString private immutable _name;
 -    ShortString private immutable _version;
+-
+-    // IMPORTANT: Deprecated. Kept to preserve the storage layout of inheriting contracts used as an
+-    // implementation behind a proxy.
 -    // slither-disable-next-line constable-states
 -    string private _nameFallback;
+-
+-    // IMPORTANT: Deprecated. Kept to preserve the storage layout of inheriting contracts used as an
+-    // implementation behind a proxy.
 -    // slither-disable-next-line constable-states
 -    string private _versionFallback;
 +    string private _name;
 +    string private _version;
 
      /**
       * @dev Initializes the domain separator and parameter caches.
-@@ -73,32 +54,22 @@ abstract contract TIP712 is ITRC5267 {
+@@ -83,32 +54,22 @@ abstract contract TIP712 is ITRC5267 {
       * contract upgrade].
       */
      constructor(string memory name, string memory version) {
--        _name = name.toShortStringWithFallback(_nameFallback);
--        _version = version.toShortStringWithFallback(_versionFallback);
+-        _name = name.toShortString();
+-        _version = version.toShortString();
 -        _hashedName = keccak256(bytes(name));
 -        _hashedVersion = keccak256(bytes(version));
 -
@@ -141,27 +151,25 @@ index 5446d00..13b4175 100644
      }
 
      /**
-@@ -149,22 +120,38 @@ abstract contract TIP712 is ITRC5267 {
+@@ -159,20 +120,38 @@ abstract contract TIP712 is ITRC5267 {
      /**
       * @dev The name parameter for the TIP712 domain.
       *
--     * NOTE: By default this function reads _name which is an immutable value.
--     * It only reads from storage if necessary (in case the value is too large to fit in a ShortString).
+-     * NOTE: This function reads `_name`, which is an immutable value.
 +     * NOTE: This function reads from storage by default, but can be redefined to return a constant value if gas costs
 +     * are a concern.
       */
 -    // solhint-disable-next-line func-name-mixedcase
 -    function _TIP712Name() internal view returns (string memory) {
--        return _name.toStringWithFallback(_nameFallback);
+-        return _name.toString();
 +    function _TIP712Name() internal view virtual returns (string memory) {
 +        return _name;
      }
 
      /**
       * @dev The version parameter for the TIP712 domain.
       *
--     * NOTE: By default this function reads _version which is an immutable value.
--     * It only reads from storage if necessary (in case the value is too large to fit in a ShortString).
+-     * NOTE: This function reads `_version`, which is an immutable value.
 +     * NOTE: This function reads from storage by default, but can be redefined to return a constant value if gas costs
 +     * are a concern.
 +     */
@@ -185,48 +193,82 @@ index 5446d00..13b4175 100644
       */
 -    // solhint-disable-next-line func-name-mixedcase
 -    function _TIP712Version() internal view returns (string memory) {
--        return _version.toStringWithFallback(_versionFallback);
+-        return _version.toString();
 +    function _TIP712VersionHash() internal view returns (bytes32) {
 +        return keccak256(bytes(_TIP712Version()));
      }
  }
 diff --git a/remappings.txt b/remappings.txt
-index 304d138..0ae9093 100644
+index 304d138..7246f36 100644
 --- a/remappings.txt
 +++ b/remappings.txt
 @@ -1 +1,2 @@
 -@openzeppelin/contracts/=contracts/
 +@openzeppelin/tron-contracts-upgradeable/=contracts/
 +@openzeppelin/tron-contracts/=lib/tron-contracts/contracts/
 diff --git a/test/utils/cryptography/TIP712.test.js b/test/utils/cryptography/TIP712.test.js
-index a445b9b..6f7fa7a 100644
+index abb2d3c..27cb6f2 100644
 --- a/test/utils/cryptography/TIP712.test.js
 +++ b/test/utils/cryptography/TIP712.test.js
-@@ -47,27 +47,6 @@ describe('TIP712', function () {
-           const rebuildDomain = await getDomain(this.eip712);
-           expect(rebuildDomain).to.be.deep.equal(this.domain);
-         });
--
--        if (shortOrLong === 'short') {
--          // Long strings are in storage, and the proxy will not be properly initialized unless
--          // the upgradeable contract variant is used and the initializer is invoked.
+@@ -39,22 +39,6 @@ describe('TIP712', function () {
+         const rebuildDomain = await getDomain(this.eip712);
+         expect(rebuildDomain).to.be.deep.equal(this.domain);
+       });
 -
--          it('adjusts when behind proxy', async function () {
--            const factory = await ethers.deployContract('$Clones');
+-      it('adjusts when behind proxy', async function () {
+-        const factory = await ethers.deployContract('$Clones');
 -
--            const clone = await factory
--              .$clone(this.eip712)
--              .then(tx => tx.wait())
--              .then(receipt => receipt.logs.find(ev => ev.fragment.name == 'return$clone_address').args.instance)
--              .then(address => ethers.getContractAt('$TIP712Verifier', address));
+-        const clone = await factory
+-          .$clone(this.eip712)
+-          .then(tx => tx.wait())
+-          .then(receipt => receipt.logs.find(ev => ev.fragment.name == 'return$clone_address').args.instance)
+-          .then(address => ethers.getContractAt('$TIP712Verifier', address));
 -
--            const expectedDomain = { ...this.domain, verifyingContract: clone.target };
--            expect(await getDomain(clone)).to.be.deep.equal(expectedDomain);
+-        const expectedDomain = { ...this.domain, verifyingContract: clone.target };
+-        expect(await getDomain(clone)).to.be.deep.equal(expectedDomain);
 -
--            const expectedSeparator = await domainSeparator(expectedDomain);
--            expect(await clone.$_domainSeparatorV4()).to.equal(expectedSeparator);
--          });
--        }
-       });
+-        const expectedSeparator = await domainSeparator(expectedDomain);
+-        expect(await clone.$_domainSeparatorV4()).to.equal(expectedSeparator);
+-      });
+     });
+
+     it('hash digest', async function () {
+@@ -90,18 +74,30 @@ describe('TIP712', function () {
+   });
 
-       it('hash digest', async function () {
+   describe('with long name and version', function () {
+-    it('deployment fails with long name', async function () {
++    it('upgradeable version supports long name', async function () {
+       const longName = 'A'.repeat(32);
+-      await expect(ethers.deployContract('$TIP712Verifier', [longName, version]))
+-        .to.be.revertedWithCustomError(this.eip712, 'StringTooLong')
+-        .withArgs(longName);
++      const instance = await ethers.deployContract('$TIP712Verifier', [longName, version]);
++
++      expect(await instance.$_TIP712Name()).to.equal(longName);
++      expect(await instance.$_TIP712Version()).to.equal(version);
++      expect(await getDomain(instance)).to.be.deep.equal({
++        ...this.domain,
++        name: longName,
++        verifyingContract: instance.target,
++      });
+     });
+
+-    it('deployment fails with long version', async function () {
++    it('upgradeable version supports long version', async function () {
+       const longVersion = 'B'.repeat(32);
+-      await expect(ethers.deployContract('$TIP712Verifier', [name, longVersion]))
+-        .to.be.revertedWithCustomError(this.eip712, 'StringTooLong')
+-        .withArgs(longVersion);
++      const instance = await ethers.deployContract('$TIP712Verifier', [name, longVersion]);
++
++      expect(await instance.$_TIP712Name()).to.equal(name);
++      expect(await instance.$_TIP712Version()).to.equal(longVersion);
++      expect(await getDomain(instance)).to.be.deep.equal({
++        ...this.domain,
++        version: longVersion,
++        verifyingContract: instance.target,
++      });
+     });
+   });
+ });