feat: implement `CommunityTokenDeployer` contract #2

0x-r4bbit · 2023-08-30T13:18:55Z

This commit introduces the CommunityTokenDeployer contract discussed in status-im/status-desktop#11954.

The idea is that, instead of having accounts deploy OwnerToken and MasterToken directly, they'd use a deployer contract instead, which maintains a registry of known OwnerToken addresses, mapped to Status community addresses.

The following changes have been made:

`OwnerToken` no longer instantiates `MasterToken`

It was, and still is, a requirement that both, OwnerToken and MasterToken are deployed within a single transaction, so that when something goes wrong, we don't end up in an inconsistent state.

That's why OwnerToken used to instantiated MasterToken and required all of its constructor arguments as well.

Unfortunately, this resulted in compilation issues in the context of the newly introduce deployer contract, where there are too many function arguments.

Because we now delegate deployment to a dedicated contract, we can instantiate both OwnerToken and MasterToken in a single transaction, without having OwnerToken being responsible to instantiate MasterToken.

This fixes the compilation issues and simplifies the constructor of OwnerToken.

Deployments via `CommunityTokenDeployer` contract

The new CommunityTokenDeployer contract is now responsble for deploying the aforementioned tokens and ensures that they are deployed within a single transaction.

To deploy an OwnerToken and MasterToken accounts can now call CommunityDeloyerToken.deploy(TokenConfig, TokenConfig, DeploymentSignature).

The DeploymentSignature uses EIP712 structured type hash data to let the contract verify that the deployer is allowed to deploy the contracts on behalf of a community account.

0x-r4bbit · 2023-08-30T13:19:23Z

@gravityblast this is a first version that I've put up for early feedback.

0x-r4bbit · 2023-08-30T13:21:05Z

contracts/CommunityTokenDeployer.sol

+                || bytes(_masterToken.symbol).length == 0 || bytes(_masterToken.baseURI).length == 0
+        ) {
+            revert CommunityTokenDeployer_InvalidTokenMetadata();
+        }


Although EIP721 token metadata is optional, I think it's still good to require it here as Status clients make heavy use of metadata.

0x-r4bbit · 2023-08-30T13:24:41Z

contracts/CommunityTokenDeployer.sol

+    function _verifySignature(DeploymentSignature calldata signature) internal view returns (bool) {
+        bytes32 digest = _hashTypedDataV4(
+            keccak256(
+                abi.encode(DEPLOYMENT_SIGNATURE_TYPEHASH, signature.signer, signature.deployer, signature.deadline)


Typically, we'd want to maintain and a put a signer nonce in here to protect against signature replay attacks.
However, we already ensure in earlier checks that only one contract can be deployed for a given community.
So if a signature was successfully used, even if it was used for a replay attack, the contract will throw an error.

0x-r4bbit · 2023-08-30T13:25:39Z

contracts/OwnerToken.sol

        address[] memory addresses = new address[](1);
-        addresses[0] = msg.sender;
+        // addresses[0] = msg.sender;
+        addresses[0] = _receiver;


msg.sender will now be the CommunityTokenDeployer contract, so we have to pass receiver separately, which will be the deployer of the contracts.

contracts/CommunityTokenDeployer.sol

0x-r4bbit · 2023-08-30T13:56:20Z

CI is not executed in this PR because the base branch is #1 .
Once #1 lands, this PR will be run against CI.

0x-r4bbit · 2023-09-04T10:58:12Z

CI has uncovered an issue with this contract.
The ContractTokenDeployer size is too big. See the statistics below:

| Contract                    | Size (kB) | Margin (kB) |
|-----------------------------|-----------|-------------|
| Address                     | 0.045     | 24.531      |
| CollectibleV1               | 10.261    | 14.315      |
| CommunityERC20              | 4.468     | 20.108      |
| CommunityOwnerTokenRegistry | 1.621     | 22.955      |
| CommunityTokenDeployer      | 32.12     | -7.544      |   // <-- this one is a problem
| Counters                    | 0.045     | 24.531      |
| ECDSA                       | 0.045     | 24.531      |
| ERC20                       | 2.802     | 21.774      |
| ERC721                      | 5.62      | 18.956      |
| MasterToken                 | 10.261    | 14.315      |
| Math                        | 0.045     | 24.531      |
| OwnerToken                  | 10.959    | 13.617      |
| ShortStrings                | 0.045     | 24.531      |
| SignedMath                  | 0.045     | 24.531      |
| StdStyle                    | 0.045     | 24.531      |
| StorageSlot                 | 0.045     | 24.531      |
| Strings                     | 0.045     | 24.531      |
| console                     | 0.045     | 24.531      |
| console2                    | 0.045     | 24.531      |
| safeconsole                 | 0.045     | 24.531      |
| stdError                    | 0.672     | 23.904      |
| stdJson                     | 0.045     | 24.531      |
| stdMath                     | 0.045     | 24.531      |
| stdStorage                  | 0.045     | 24.531      |
| stdStorageSafe              | 0.045     | 24.531      |

The issue is most likely that it includes the source of OwnerToken as well as MasterToken which are both ultimately ERC721 tokens. So it has both of those sources in addition to its own source (which actually isn't that big). We can see that by inspecting the sizes of OwnerToken and MasterToken which are both (~10kb), so that already makes up ~20kb of the ~32kb of CommunityTokenDeployer.

OwnerToken and MasterToken happen to be fairly big in size compared to all other code we need anywias. Need to check why that is because even the EIP721 implementation is ~6kb in size.

I need to think about how to overcome this.

One thing that comes to mind atm is that we use the Minimal Proxy Contract pattern, which would require separate deployment of OwnerToken and MasterToken templates, and CommunityTokenDeployer to only create tiny proxy clones, which removes the requirement of having all the sources available at deployment.

Another option would be to look into other dependency sources and see if we can extract only the bits and pieces we need, however, if we look at the sizes of library dependencies we have (e.g ECDSA and Counter and ERC721), those are fairly small in comparison.

A third option I see is exploring what shared functionality of BaseToken is really necessary, and whether we should not have OwnerToken and MasterToken depend on it, but both having their own implementation with only the stuff that's needed.

0x-r4bbit · 2023-09-04T11:59:22Z

Did a quick experiment and removed the dependency of BaseToken in OwnerToken as most of what BaseToken does is not actually needed by OwnerToken (like being able to set maxSupply, remote burnable and transferable checks, and being an ERC721Enumerable).

I could reduce the contract size of OwnerToken by ~40%:

| Contract                    | Size (kB) | Margin (kB) |
|-----------------------------|-----------|-------------|
| Address                     | 0.045     | 24.531      |
| CollectibleV1               | 10.477    | 14.099      |
| CommunityERC20              | 4.652     | 19.924      |
| CommunityOwnerTokenRegistry | 1.621     | 22.955      |
| CommunityTokenDeployer      | 26.311    | -1.735      |
| Counters                    | 0.045     | 24.531      |
| ECDSA                       | 0.045     | 24.531      |
| ERC20                       | 2.802     | 21.774      |
| ERC721                      | 5.62      | 18.956      |
| MasterToken                 | 10.477    | 14.099      |
| Math                        | 0.045     | 24.531      |
| OwnerToken                  | 6.525     | 18.051      | <-- ~40% smaller
| ShortStrings                | 0.045     | 24.531      |
| SignedMath                  | 0.045     | 24.531      |
| StdStyle                    | 0.045     | 24.531      |
| StorageSlot                 | 0.045     | 24.531      |
| Strings                     | 0.045     | 24.531      |
| console                     | 0.045     | 24.531      |
| console2                    | 0.045     | 24.531      |
| safeconsole                 | 0.045     | 24.531      |
| stdError                    | 0.672     | 23.904      |
| stdJson                     | 0.045     | 24.531      |
| stdMath                     | 0.045     | 24.531      |
| stdStorage                  | 0.045     | 24.531      |
| stdStorageSafe              | 0.045     | 24.531      |

Tests are still green. In fact, I could remove a few for OwnerToken as we no longer need to check for remoteBurnable() and alike.

This was just a quick experiment and I'd like to double check this with @gravityblast before proposing these changes.
We can possibly squeeze out some bytes of MasterToken and CollectibleV1 as well (which both depend on BaseToken, hence the shared big size).

Right now, even with the changes I've done CommunityTokenDeployer is still ~1.7kb too big.

0x-r4bbit · 2023-09-04T15:16:10Z

Update:

In another discussion with @3esmit, he threw in the option to introduce factory contracts for both OwnerToken and MasterToken and have CommunityTokenDeployer reference the two.
That way we extract the big sources out of CommunityTokenDeployer at the cost of needing to deploy two more factory contracts.

This can then be combined with additional minimal contract proxy pattern to squeeze out more gas.

Will explore these ideas and update the PR accordingly.

0x-r4bbit · 2023-09-05T12:59:39Z

Following up on the exploration described above, after introducing a factory for OwnerToken and MasterToken and having those referenced and used in the deployer contract, we're reducing the size of the deployer contract by 18% as expected.

Factory contracts now carry the biggest baggage.
This solution already does the trick, but we can take this one step further and reduce gas costs for deployments significantly using minimal contract proxies.

| Contract                    | Size (kB) | Margin (kB) |
|-----------------------------|-----------|-------------|
| Address                     | 0.045     | 24.531      |
| CollectibleV1               | 10.261    | 14.315      |
| CommunityERC20              | 4.468     | 20.108      |
| CommunityMasterTokenFactory | 13.495    | 11.081      |
| CommunityOwnerTokenFactory  | 16.937    | 7.639       |
| CommunityOwnerTokenRegistry | 1.621     | 22.955      |
| CommunityTokenDeployer      | 5.863     | 18.713      |
| Counters                    | 0.045     | 24.531      |
| ECDSA                       | 0.045     | 24.531      |
| ERC20                       | 2.802     | 21.774      |
| ERC721                      | 5.62      | 18.956      |
| MasterToken                 | 10.261    | 14.315      |
| Math                        | 0.045     | 24.531      |
| OwnerToken                  | 10.959    | 13.617      |
| ShortStrings                | 0.045     | 24.531      |
| SignedMath                  | 0.045     | 24.531      |
| StdStyle                    | 0.045     | 24.531      |
| StorageSlot                 | 0.045     | 24.531      |
| Strings                     | 0.045     | 24.531      |
| console                     | 0.045     | 24.531      |
| console2                    | 0.045     | 24.531      |
| safeconsole                 | 0.045     | 24.531      |
| stdError                    | 0.672     | 23.904      |
| stdJson                     | 0.045     | 24.531      |
| stdMath                     | 0.045     | 24.531      |
| stdStorage                  | 0.045     | 24.531      |
| stdStorageSafe              | 0.045     | 24.531      |

0x-r4bbit · 2023-09-05T14:12:36Z

@3esmit I've updated the pull request such that it includes a CommunityMasterTokenFactory and a CommunityOwnerTokenFactory which are referenced by CommunityTokenDeployer.

As described in the comment above, that alone enables us to get past the ~24kb contract size limit.

I've added tests and docs accordingly.
Next step would be to extend the factories to use minimal contract proxies, although that'd primarily to decrease gas costs per deployment.

I'd appreciate if you could take a look here and leave any feedback you might have!

0x-r4bbit · 2023-09-06T12:06:24Z

Looking into using EIP1167 in the token factories to make deployment of tokens cheaper. Then I realized this conflicts with how the token contracts need to be initialized at the moment.

For EIP1167 to work, we'll have to deploy OwnerToken and MasterToken as templates so they can be cloned from there. But both of these tokens need constructor arguments that we don't have at the time of putting them out as templates.

Unless we want to rewrite how these tokens work completely, I'm not sure it makes sense to further pursue that idea.

gravityblast · 2023-09-19T08:13:27Z

LGTM!

This commit introduces the `CommunityTokenDeployer` contract discussed in status-im/status-desktop#11954. The idea is that, instead of having accounts deploy `OwnerToken` and `MasterToken` directly, they'd use a deployer contract instead, which maintains a registry of known `OwnerToken` addresses, mapped to Status community addresses. The following changes have been made: It was, and still is, a requirement that both, `OwnerToken` and `MasterToken` are deployed within a single transaction, so that when something goes wrong, we don't end up in an inconsistent state. That's why `OwnerToken` used to instantiated `MasterToken` and required all of its constructor arguments as well. Unfortunately, this resulted in compilation issues in the context of the newly introduce deployer contract, where there are too many function arguments. Because we now delegate deployment to a dedicated contract, we can instantiate both `OwnerToken` and `MasterToken` in a single transaction, without having `OwnerToken` being responsible to instantiate `MasterToken`. This fixes the compilation issues and simplifies the constructor of `OwnerToken`. The new `CommunityTokenDeployer` contract is now responsble for deploying the aforementioned tokens and ensures that they are deployed within a single transaction. To deploy an `OwnerToken` and `MasterToken` accounts can now call `CommunityDeloyerToken.deploy(TokenConfig, TokenConfig, DeploymentSignature)`. The `DeploymentSignature` uses `EIP712` structured type hash data to let the contract verify that the deployer is allowed to deploy the contracts on behalf of a community account.

0x-r4bbit · 2023-09-19T09:38:43Z

contracts/CommunityTokenDeployer.sol

+        uint8 v;
+        bytes32 r;
+        bytes32 s;
+    }


@gravityblast I have removed the deadline field here.

@endulab has correctly pointed out that, given that the deployer creates the signature on behalf of the community via the community's private key, she can also set a deadline that far in the future, which renders the deadline useless.

Unless only the community controls the signature creation there's very little benefit in having it.

0x-r4bbit commented Aug 30, 2023

View reviewed changes

contracts/CommunityTokenDeployer.sol Show resolved Hide resolved

0x-r4bbit force-pushed the feat/community-token-deployer branch from 4007ad6 to 94a80bb Compare August 30, 2023 13:52

0x-r4bbit force-pushed the feat/community-token-deployer branch from 94a80bb to 4327202 Compare August 30, 2023 15:18

0x-r4bbit changed the title ~~WIP: implement CommunityTokenDeployer contract~~ feat: implement CommunityTokenDeployer contract Aug 30, 2023

0x-r4bbit force-pushed the feat/community-token-deployer branch 3 times, most recently from 0ae5c28 to f14b1f4 Compare August 31, 2023 10:22

0x-r4bbit force-pushed the refactor/foundry-template branch 2 times, most recently from 6fe5791 to 69eda28 Compare August 31, 2023 10:29

0x-r4bbit force-pushed the feat/community-token-deployer branch from f14b1f4 to 7f5d8f7 Compare August 31, 2023 10:46

0x-r4bbit force-pushed the refactor/foundry-template branch from 69eda28 to 74e0bb9 Compare August 31, 2023 10:49

0x-r4bbit changed the base branch from refactor/foundry-template to main August 31, 2023 10:50

0x-r4bbit changed the base branch from main to refactor/foundry-template August 31, 2023 10:50

0x-r4bbit mentioned this pull request Aug 31, 2023

[Community Ownership] deploy Owner Token through Deployer contract status-im/status-desktop#11954

Closed

2 tasks

0x-r4bbit requested a review from gravityblast September 1, 2023 09:34

0x-r4bbit force-pushed the refactor/foundry-template branch 2 times, most recently from cd1e184 to 0c4e1d1 Compare September 4, 2023 10:34

0x-r4bbit force-pushed the feat/community-token-deployer branch from 7f5d8f7 to 770e715 Compare September 4, 2023 10:37

0x-r4bbit closed this Sep 4, 2023

0x-r4bbit reopened this Sep 4, 2023

0x-r4bbit force-pushed the feat/community-token-deployer branch from 770e715 to e7e7221 Compare September 4, 2023 11:22

0x-r4bbit requested a review from 3esmit September 4, 2023 11:59

0x-r4bbit force-pushed the feat/community-token-deployer branch from e7e7221 to 070ac11 Compare September 5, 2023 14:03

0x-r4bbit force-pushed the refactor/foundry-template branch 2 times, most recently from b4aaee5 to 5f0974c Compare September 5, 2023 14:39

0x-r4bbit force-pushed the feat/community-token-deployer branch from 070ac11 to 77d8504 Compare September 5, 2023 14:41

0x-r4bbit mentioned this pull request Sep 5, 2023

Consider use of Ownable2Step over Ownable #7

Closed

0x-r4bbit force-pushed the feat/community-token-deployer branch from 77d8504 to b9e0ebc Compare September 6, 2023 10:39

0x-r4bbit force-pushed the refactor/foundry-template branch 2 times, most recently from 98912f0 to c2f500c Compare September 8, 2023 10:36

0x-r4bbit force-pushed the feat/community-token-deployer branch 2 times, most recently from 61c79a5 to 0602ce4 Compare September 13, 2023 07:34

Base automatically changed from refactor/foundry-template to main September 19, 2023 08:12

gravityblast approved these changes Sep 19, 2023

View reviewed changes

0x-r4bbit force-pushed the feat/community-token-deployer branch from 0602ce4 to 5542cc0 Compare September 19, 2023 09:36

0x-r4bbit commented Sep 19, 2023

View reviewed changes

0x-r4bbit merged commit 4be8613 into main Sep 19, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement `CommunityTokenDeployer` contract #2

feat: implement `CommunityTokenDeployer` contract #2

0x-r4bbit commented Aug 30, 2023

0x-r4bbit commented Aug 30, 2023

0x-r4bbit Aug 30, 2023

0x-r4bbit Aug 30, 2023

0x-r4bbit Aug 30, 2023 •

edited

Loading

0x-r4bbit commented Aug 30, 2023

0x-r4bbit commented Sep 4, 2023 •

edited

Loading

0x-r4bbit commented Sep 4, 2023

0x-r4bbit commented Sep 4, 2023

0x-r4bbit commented Sep 5, 2023

0x-r4bbit commented Sep 5, 2023

0x-r4bbit commented Sep 6, 2023

gravityblast commented Sep 19, 2023

0x-r4bbit Sep 19, 2023

feat: implement CommunityTokenDeployer contract #2

feat: implement CommunityTokenDeployer contract #2

Conversation

0x-r4bbit commented Aug 30, 2023

OwnerToken no longer instantiates MasterToken

Deployments via CommunityTokenDeployer contract

0x-r4bbit commented Aug 30, 2023

0x-r4bbit Aug 30, 2023

Choose a reason for hiding this comment

0x-r4bbit Aug 30, 2023

Choose a reason for hiding this comment

0x-r4bbit Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

0x-r4bbit commented Aug 30, 2023

0x-r4bbit commented Sep 4, 2023 • edited Loading

0x-r4bbit commented Sep 4, 2023

0x-r4bbit commented Sep 4, 2023

0x-r4bbit commented Sep 5, 2023

0x-r4bbit commented Sep 5, 2023

0x-r4bbit commented Sep 6, 2023

gravityblast commented Sep 19, 2023

0x-r4bbit Sep 19, 2023

Choose a reason for hiding this comment

feat: implement `CommunityTokenDeployer` contract #2

feat: implement `CommunityTokenDeployer` contract #2

`OwnerToken` no longer instantiates `MasterToken`

Deployments via `CommunityTokenDeployer` contract

0x-r4bbit Aug 30, 2023 •

edited

Loading

0x-r4bbit commented Sep 4, 2023 •

edited

Loading