A0-3579: add admin to wrapped token contract #39
Conversation
azero/contracts/psp22/lib.rs
Outdated
| impl Burnable for Token { | ||
| #[ink(message)] | ||
| fn burn(&mut self, from: AccountId, value: u128) -> Result<(), PSP22Error> { | ||
| self.ensure_admin()?; |
There was a problem hiding this comment.
IIRC, the Membrane contract code calls burn with from: self.env().caller(). This will work, if we set that contract to be a an admin of PSP22 token, but I'd think that a cleaner way would be to call transfer_from(caller, self.env().account_id()) (i.e. first transfer tokens back to the bridge contract) and then burn them. Rather than directly burning tokens in the user's pocket.
Just a food for thought.
There was a problem hiding this comment.
Yeah, that makes more sense to me as well. I've heard an argument raised that "it's more operations, so it's more expensive" - how much more?
There was a problem hiding this comment.
Final 2 cents:
In DEX, the way similar mechanics looks like is the following: when user wants to trade/lock tokens in the DEX the following happens:
- User increases the allowance for the DEX contract to spend the token
- User calls DEX's endpoint (swap/provide liquidity)
- DEX calls
transfer_from(caller, self, amount) - DEX does its things
In here, you'd replace DEX with Bridge contract and does its things would be PSP22::burn. So, in my mind, the bridge contract should first transfer_from(caller, self) the tokens and then proceed. Not transfer within the burn.
Although, one my consider this to be a special case where there is actual owner of the token that can do anything. In my mind that's too much power and we would be flagged by every auditor or researcher (like l2beats) if they saw this code.
That being said, it is true that interacting with a contract like this will be more expensive. There's not only 1 additional transfer_from call but the user (caller) will need to increase the allowance before burning(bridging) the tokens.
There was a problem hiding this comment.
my 2 dollars 7 cents:
we agreed (cc'ing @h4nsu) to add optional hooks to the psp22 implementation: https://cardinal-cryptography.atlassian.net/browse/A0-3600. For these tokens the hooks will check whether owner approved the spender to burn the amount of tokens from his account. This is the ERC20 behaviour and we reckon it was omitted from the standard, by mistake.
There was a problem hiding this comment.
If that's agreed to be added to PSP22, then I would guess that the scope of this PR should be reduced to ensuring that only the admin can mint (so I should remove changes to burn)?
There was a problem hiding this comment.
I'd say so, but maybe call the roles explicitly Minter and Burner. Admin should be a role reserved to e.g. updating the contract code, setting the roles etc
There was a problem hiding this comment.
Hmm, I guess that we want to have a safeguard against user burning own tokens without interaction with bridge contract anyway, so I would actually leave the original version for now?
When changes to PSP22 are made then we will have both check for being burned by the contract and for sufficient allowance
There was a problem hiding this comment.
the role check will not change anyway, so why not do it now given a chance, but your call, resolve at will
krzysztofziobro
force-pushed
the
A0-3579-token-roles
branch
from
1d84c8a to
d9e38ea
Compare
Adds admin role to token contract: it can mint/burn tokens + added unit tests for token.
Also, had to update our ink_e2e tests, which resulted in a boilerplate-reducing refactor.