Add transferStakeFrom and approve functions to staking precompile#2478
Conversation
| } | ||
|
|
||
| #[precompile::public("increaseAllowance(bytes32,uint256,uint256)")] | ||
| fn increase_allowance( |
There was a problem hiding this comment.
We can use approve interface to do increase/decrease allowance, and they are not in the standard ERC20 definition.
There was a problem hiding this comment.
Contracts can easily get the current allowance and increase it atomically (they must check for re-entrancy attacks), however users cannot do that. That's why those functions are very common in tokens implementing ERC20.
|
It is better to implement both function in an Alpha wrapper contract, instead of in precompile. @ppolewicz @camfairchild any update for ERC20-alpha-wrapper |
precompiles/src/staking.rs
Outdated
| <R as frame_system::Config>::AccountId, | ||
| // For each pair of (spender, netuid) | ||
| Blake2_128Concat, | ||
| (<R as frame_system::Config>::AccountId, u16), |
There was a problem hiding this comment.
What happens on subnet de-reg?
There was a problem hiding this comment.
I'm not sure a de-reg would cause any issue. There approve would still be here but the user can update the allowance at anytime, and is suppose to trust/verify that the contract will do appropriate things with the approved funds. So I think it is not worth it to complicate the design.
There was a problem hiding this comment.
After de-reg, we need clean up the data, the transfer alpha doesn't work for de-reg netuid.
And for approved allowance, there are two options.
- just clean up the storage
- transfer alpha according to each allowance with the same netuid.
There was a problem hiding this comment.
Clearing the storing with the netuid prefix is a bad idea because there can be a lot of entries, and it is not safe in general to have unbounded storage storage writes.
If a netuid is de-reg, calling transferStakeFrom will revert like transferStake even if there is allowance, because under the hood they call the same dispatchable. Once the netuid is registered again however, the transfers will work again (if user have funds in the new alpha), and allowance will be preserved.
For the last point (allowance preservation), I think this is not the issue as stated in my previous comment. If the user trust (or verify code) the contract with a long-live high amount allowance to not be malicious and transfer the funds in unwanted way, there is no reason the contract will behave differently after the de-reg/re-reg. If one think a contract may go rogue in the future, they should either not interact with it or make a short-live exact amount approval and consume it quickly.
In any case at the protocol level I don't see any risk of keeping the allowances across netuid de-regs, as they don't interact directly with the staking pallet, and instead ends up being a normal transferStake call (on the behalf of the approver) only when the spender call transferStakeFrom.
Do you see a concrete issue I missed?
There was a problem hiding this comment.
If a subnet is de-registered, its netuid will be locked and no one can use it again before all data are cleaned up. But it could be reused again after all old data removed from the storage. So the new registered subnet is different one even the netuid is the same, means the alpha is not the same.
There was a problem hiding this comment.
Clearing big storage maps is risky in Substrate in general :/
For the approvals as said above it's not really a problem imo that they stay across netuid de-reg/re-reg. Users trust the smart contract to not consume the approval without a valid reason.
Approvals are just numbers which allows transfering stake on the behalf of the approver. After de-reg/re-reg, the approver will have no stake and the transferFrom will fail because the transfer part itself will fail in the pallet (like if you called yourself transfer). If they stake for this new network with same netuid, their previous approval will still be there and transfer could be made indeed, but they already trusted the contract so it should not be an issue. If they don't trust the contract long-term, they should like with ERC20 only approve the required amount to be consumed in a short timeframe, returning the allowance back to 0.
So I really so no issue of keeping the allowance data in storage.
|
please merge devnet-ready, will fix the failed CI tasks. |
4 participants
Description
Adds new functions to the staking precompile similar to ERC20
transferFromandapprove, which will allow smart contracts to atomically receive alpha tokens and react to the transfer, which is not possible today (if an user transfers alpha token to a smart contract, the latter is not aware of this transfer). Allowance is managed per netuid for finer control.Related Issue(s)
None
Type of Change
Breaking Change
None
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Not applicable
Additional Notes
None