runtime-sdk: Implement ERC-20 compatibility #2144
Conversation
✅ Deploy Preview for oasisprotocol-oasis-sdk canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2144 +/- ##
==========================================
- Coverage 58.88% 57.65% -1.23%
==========================================
Files 144 146 +2
Lines 10172 10451 +279
==========================================
+ Hits 5990 6026 +36
- Misses 4182 4425 +243 ☔ View full report in Codecov by Sentry. |
runtime-sdk/modules/evm/src/test.rs
Outdated
| const DECIMALS: u8 = 18; | ||
| } | ||
| Some(BTreeMap::from([( | ||
| primitive_types::H160([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]), |
There was a problem hiding this comment.
I know this is just a test but should there be some additional checks that the addresses here cannot be all zeroes in practice (or any other reserved address).
| const DECIMALS: u8 = T::DECIMALS; | ||
|
|
||
| fn total_supply() -> Result<u128, Error> { | ||
| let denom = token::Denomination::from_str(Self::SYMBOL).unwrap(); |
There was a problem hiding this comment.
Would we ever want to have a different symbol than the denomination name? I think we might want to use symbol "ROSE"/"Native ROSE" (or "TEST" on testnet) and not "NATIVE" for the native denomination mapping?
| Ok(T::Accounts::get_total_supplies()? | ||
| .get(&denom) | ||
| .copied() | ||
| .unwrap_or_default()) |
There was a problem hiding this comment.
Should this fail if the denomination doesn't exist?
| } | ||
|
|
||
| fn mint(to: address::Address, amount: &token::BaseUnits) -> Result<(), Error> { | ||
| T::Accounts::mint(to, amount) |
There was a problem hiding this comment.
Minting should likely not be allowed or even supported. The ERC-20 spec doesn't require this method.
We could implement an ERC20 with ownership support (e.g. https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#ERC20Mintable) but that's probably not needed at this time, since I believe the main usecase currently is exposing the native denomination via ERC20 interface.
There was a problem hiding this comment.
Minting and burning specifically are requirements for this feature. People usually seem to implement these two methods on top of the base ERC-20 spec so I added them, but we can decide which particular interface to follow.
There was a problem hiding this comment.
Okay, that makes sense. But I believe that in that case, we need to include a configurable ownership mechanism as well. Since without it, everyone will be able to call this method and mint tokens?
jberci
force-pushed
the
jberci/feature/erc20
branch
from
1c7fc23 to
14d3b9d
Compare
jberci
force-pushed
the
jberci/feature/erc20
branch
4 times, most recently
from
bfa8638 to
3bfe29b
Compare
| ) -> Result<bool, Error> { | ||
| with_allowance::<Self, _>(owner, spender, |allowance| { | ||
| let allowance = allowance.unwrap_or_default(); | ||
| if amount.amount() <= allowance { |
There was a problem hiding this comment.
I don't think this is defined in the official spec, but the OpenZeppelin ERC-20 token contract treats the uint:256::max() allowance value as infinity, and doesn't deduct from it:
probably doesn't have an effect in practice.
There was a problem hiding this comment.
There's an issue here that amounts in EVM are u256, but we use only u128 for them. I can do the equivalent of infinity but for u128, not sure how wrong that would be.
| if amount.amount() <= allowance { | ||
| Ok(allowance - amount.amount()) | ||
| } else { | ||
| Err(Error::InsufficientBalance) |
There was a problem hiding this comment.
Should we follow error conventions as defined in https://eips.ethereum.org/EIPS/eip-6093
Not sure if it matters in practice.
| @@ -1201,3 +1207,527 @@ fn test_return_value_limits() { | |||
| assert_eq!(result[64], 0xFF, "result should be correct"); | |||
| assert_eq!(result[1023], 0x42, "result should be correct"); | |||
| } | |||
|
|
|||
| #[derive(Default)] | |||
| pub struct TestErcToken {} | |||
There was a problem hiding this comment.
This is maybe more for a follow-up, potentially it should go even in a separate repo (sapphire-paratime?), but we should consider running some ERC-20 token test cases against our precompile implementation?
e.g. https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/test/token/ERC20
jberci
force-pushed
the
jberci/feature/erc20
branch
from
3bfe29b to
d9a4e42
Compare
Implements #1856.