Mint restriction: Signature-based minting

liteflow-labs / rarible-protocol-contracts

Interfaces for smart contracts used by Rarible

MIT License

0 stars 0 forks source link

Mint restriction: Signature-based minting #3

Open NicolasMahe opened 2 years ago

NicolasMahe commented 2 years ago

Implement a signature-based minting so any backend can sign a specific authorization to their users.

NicolasMahe commented 2 years ago

After discussion with Hadrien, we decide to implement it the following way:

Reuse MinterAccessControl smart contract. It already implement the enable/disable and a list of authorized minters.
Add the operator signature at the end of the MintXXXData.signatures
If there is one more signatures than the number of creators
- then check this signature is signed by a minter from MinterAccessControl
- bypass check previously done by MinterAccessControl.isValidMinter
- Remove the signature from the signatures payload so the contract checks can continue to work
Else, execute the existing logic from MinterAccessControl.isValidMinter

NicolasMahe commented 2 years ago

Not to do in the same PR but nice to have: The function to validate the signature is not optimised for the use we are going to do. Would be nice to check how much less gas we would get with like 3 creators and 5 minters

Hadrienlc commented 2 years ago

If there is one more signatures than the number of creators

then check this signature is signed by a minter from MinterAccessControl

Unfortunately, it's not possible to iterate mapping (address => bool) private _minters; Should we go back to a separate class implementation than MinterAccessControl with only 1 address operator to check the signature against ?

NicolasMahe commented 2 years ago

Should we go back to a separate class implementation than MinterAccessControl with only 1 address operator to check the signature against ?

What about using an iterable map? Open zeppelin has one. The new pr could change the structure in MinterAccessControl

NicolasMahe commented 2 years ago

Another idea: use directly hash.recover to return signer address:

signerFromSig = hash.recover(signature);

source: https://github.com/rarible/protocol-contracts/blob/master/tokens/contracts/erc-1271/ERC1271Validator.sol

Less code reusability, but much simpler and gas-efficient

Hadrienlc commented 2 years ago

signerFromSig = hash.recover(signature);

won't work for a contract, see https://github.com/rarible/protocol-contracts/blob/b1b1593686fc78ceecab123124059acd908fafe1/tokens/contracts/erc-1271/ERC1271Validator.sol#L24 requires the signer to be passed as parameter.

EnumerableMap can be a good idea but is a breaking change that will be hard to handle if the first PR is deployed before this one. It will be almost impossible to migrate the configuration from the mapping since we can't enumerate the values. Can we block the first PR to switch to an EnumerableMap on MinterAccessControl before the first release ?

NicolasMahe commented 2 years ago

EnumerableMap can be a good idea but is a breaking change that will be hard to handle if the first PR is deployed before this one. It will be almost impossible to migrate the configuration from the mapping since we can't enumerate the values. Can we block the first PR to switch to an EnumerableMap on MinterAccessControl before the first release ?

Good point. I think the first PR will not be merged or review before you finish this new PR. Implement it in a new PR with a nice dedicated commit that changes the structure and I will push it back to the PR later

NicolasMahe commented 2 years ago

Waiting for https://github.com/liteflow-labs/rarible-protocol-contracts/issues/2 to be done first