I definitely see the practical use case for the proposal though I feel that the proposed interface is too monolithic. There should be a clearer technical specification on what is required vs what is recommended, especially in terms of interfaces / events.
There are discrepancies between the proposed definitions, whitepaper and the sample implementation, so I will just focus on what was submitted on Github.
-
Does the sovToken necessary need to be a ERC 20 compatible token? I feel that is too restrictive. It’s quite possible for the native token or ERC1155 or ERC721 tokens to be store of value as well.
-
“PBM business logic” is a key part of the proposal and should be expounded. Is it restricted to access control logic or are there other parts to it? If it’s about access control, how about aligning it with ERC-5982: Role-based Access Control
-
I don’t think there’s a need to specify that PBM tokens should be ERC-1155. Why can’t they be ERC-20 and the decision of what form a PBM token should take is more of a business use case anyway.
-
“PBM MUST ensure the destination address for unwrapped sovToken is in a whitelist of Merchant/Redeemer addresses and not in a blacklist of banned addresses prior to unwrapping the underlying sovToken.” This is implementation specific e.g. do you need a blacklist, a whitelist or both. As long as the access control logic is well defined and auditable, this is again more of a use case specific implementation.
-
Implementation wise, I don’t think the PBM wrapper should be bound to the Token Manager. The ERC-20 contract can be wrapped to ERC-1155 (e.g. GitHub - 0xsequence/erc20-meta-token: General ERC20 to ERC1155 Token Wrapper Contract) first before wrapped to a PBM. This would reduce the requirement of being a PBM while making it more interoperable with existing standards out there.