-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Community Ownership] deploy Owner Token through Deployer contract #11954
Comments
Lookup signing a transaction by the community and then send the transaction with the user wallet (to pay the gas) |
It seems like if we go the route I suggested, it might make it so that the account paying the fee will still be the one marked as signer, which is not what we want, we need to find another solution |
One option I discussed with @endulab is to have a "util/singleton" contract called CommunityOwnerTokenDeployer, that would basically be a factory that would be deployed once, and we'd "hardcode" the address in status-go. Then, when deploying the Owner and TokenMaster tokens, we'd use this CommunityOwnerTokenDeployer contract instead and it would then store the addresses of the deployed token contracts in a map using the Afterwards, when a user gets sent an OwnerToken, they can ask the CommunityOwnerTokenDeployer if that token is indeed the right one for the community. The contract will check its map and send the address of the deployed token. We need to check with contract experts to know if this option is doable. I do think it solves the problem of a malicious owner trying to scam with a fake owner token. We then need to check if there are legal issues with deploying that type of CommunityOwnerTokenDeployer contract. |
If the |
Yep! (that's what I'm saying in the quoted text, maybe you misread) |
Lol, I read "I don't think..", sorry 😅 |
A few thoughts:
|
I don't think it would require an upgrade policy. If an update is needed, a new deplorer contract could just be deployed, and then we would make the Status client code aware of this new deplorer contract
I think this is probably fine, as long as the deplorer contract doesn't have an upgrade mechanism (so the only way to perform an upgrade is to deploy a new contract, and point the status client code at that new contract).
|
In this case we need to use the new and old deployer contract, because old ones already keep community <-> owner token data. Or we break backward-compatibility ? |
Hi everyone, reading this thread here and I might have some questions (because I do miss some context).
@jrainville This "they can ask the deployer token if it's indeed the right one" part - is that happening in status-go? Meaning, somehow a Status client gets notified that the logged-in account has received an owner token and now it wants to verify if it's indeed the owner token of this community? If the answer is yes, then:
^ This would become a problem. The
@John-44 if this part is indeed fine, then we might want to revisit the deployment of owner/master token altogether because there's patterns that can be used to reduce gas costs by a lot, provided Status deploys some initial contract. If it is an option, we could maybe squeeze out some more gas, but would need to do some profiling on that. |
Could a new contract reference data related to previous deplorer contracts? If so, could this be a solution to the issue of preserving the data written to the previous contract when we deploy a replacement contract? |
I think it's probably fine if Status deploys some initial contract for the owner/TokenMaster contracts only. Also any initial contract that Status deploys shouldn't have an upgrade mechinism. The contracts where Status cannot deploy an initial contract for (even if it saves gas) are:
|
Hey everyone, a small update here.
At this point, I think it's also worth mentioning that, even though the flow above would ensure the community performs the deployment and therefore one can check on-chain if a given contract address was indeed the first one deployed by the community, it doesn't actually guarantee what is being deployed. A malicious owner can still go ahead and change the source of the contract being deployed (and still have the community perform the transaction, as the owner owns the private key at this point). That being said, exploring the idea of a Here's how I think this could work:
Questions to clarify
Additional considerations
|
I think this 4.2 option is better. Like you said, 4.1 requires sending funds to the community which comes with a slew of possible problems and its not great UX.
I'm not sure. The control node has the last say (only one that can modify the community description), so normal clients don't need to access that information.
We should assume that we'll have new roles in the future. Will those roles need a special token like the TokenMaster does? I'm not sure. The admin for example can use any ERC721 or ERC20
I think having the Registry in its own contract is a good idea. It's more probable that the Deployer will change than the Registry will. |
So, as far as I understood, the idea was that any (member) node can verify that a given contract address is indeed an owner token contract address for some community. Hence we discussed the idea of a registry that maps communityID<->contractAddress. Here I'm asking if these nodes also want to verify that a token address is indeed the tokenmaster token address for that community. So, do we need communityID<->tokenMasterAddress as well?
After further discussions offline with @3esmit , we might get away with not having a registry at all, but rather have contract addresses deterministically calculated. Will have to play around with this. |
I also agree that option 4.2. is better than 4.1. 4.1 can be very problematic, especially in case of eg. not enough funds. It's hard to say what to do then, whether community should give back money to the owner or the owner should send more money to redo the transaction. Regarding tokenMaster mapping, every change made by tokenMaster should be accepted by the control node after all. Control Node knows who is tokenMaster. I am not sure if a member need to verify any operations made by token master. At the first sight it seems that we do not need to keep a mapping for token master. |
I prefer option 4.2 |
Yup, that's what I was trying to say, but I realize I wasn't being very clear 😅 |
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.
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.
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.
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.
Here's an illustration of the system as it's currently implemented in status-im/communities-contracts#2 @John-44 There'd be two contracts that need to be deployed by Status |
@0x-r4bbit looks good to me! John is off I think for the next week or so. |
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.
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.
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.
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.
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.
Hey everyone, another quick update. Turns out we ran into a contract size limit issue with deployer contract as it depends on the contract sources of both, the owner token and the master token. I've done some experiments and ultimately we went with splitting up the contracts further, such that we also have a This is what it looks like now: I've also written a bunch of documentation about this in: status-im/communities-contracts#4 |
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.
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.
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.
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.
The Owner Token must be deployed by the community address. This is to prevent malicious owners from deploying multiple Owner Tokens with potentially harmful intentions in the future.
This will effectively bind the contract to the community. Clients will verify the owner token contract address by reviewing the transaction history of the community address; only the first deployment is valid.
Edit: Use Deployer contract method described in comments below
Ignore this if the way above works
To facilitate the deployment by the community address, it needs sufficient funds to cover the gas fees. Therefore, the procedure should be:
Acceptance criteria:
Implementation:
Ideally, these four steps should be executed in a single atomic transaction. This ensures that either all operations succeed, or none are executed, preventing any intermediate and undesirable state. If executing all in one transaction is not feasible, a robust recovery/retry mechanism should be implemented in the event that any of the transactions fail.
Considerations:
Extend Owner Token's constructor with
signerPublicKey
parameter to avoid step 4.The text was updated successfully, but these errors were encountered: