-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: erc20 evm precompiles #1181
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Gregory Hill <[email protected]>
b2eec9e
to
4c8ba8c
Compare
Signed-off-by: Gregory Hill <[email protected]>
CurrencyId::LendToken(_) => { | ||
let mut vec = Vec::new(); | ||
vec.extend_from_slice(&b"q"[..]); | ||
vec.extend_from_slice(&loans::UnderlyingAssetId::<T>::get(self)?.name::<T>()?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably disallow recursive currencies in the loans pallet (i.e. underlying cannot be lend token).
// NOTE: `name`, `symbol` and `coingecko_id` are not bounded | ||
// estimate uses 32 bytes each |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can fix this once open-web3-stack/open-runtime-module-library#949 is merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest this is quite hard to review. It looks OK but I'm not at all confident that I would catch any bugs in this pr. One thing that I did wonder about, is shouldn't we have some tests of actual contracts to verify that all the glue logic works?
.. | ||
} => return IsPrecompileResult::Answer { | ||
is_precompile: true, | ||
extra_cost: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this extra cost for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming it is the extra gas used to verify the address is a precompile, Moonbeam has also set this to zero.
match self { | ||
LpToken::Token(token) => Some(token.name().as_bytes().to_vec()), | ||
LpToken::ForeignAsset(foreign_asset_id) => ForeignAssetInfo(*foreign_asset_id).name::<T>(), | ||
LpToken::StableLpToken(stable_pool_id) => StablePoolInfo(*stable_pool_id).name::<T>(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we shouldn't include stablelptoken stuff since we don't have that currently anyway? Then we'll be free to change the types if needed (admittedly unlikely)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any tokens so I think it is fine to leave as-is for now.
Adds ERC20 compatible precompile(s) for all currencies at well-known addresses, currently derived using their SCALE encoding.
Our approach is similar to the architecture adopted by Moonbeam, the precompile(s) are compliant with the ERC20 standard including event logging. Conversely, Acala uses the precompiles to solely interact with the runtime and uses pre-deployed Solidity contracts to handle everything else.
I have reused some of the utilities developed by Moonbeam here. See also their ERC20 precompile(s) for interacting with
pallet-assets
here. For reference, Acala has similar utility code here and their multicurrency precompile is here.TODO
PrecompileSet
sAllowance
,Approve
andTransferFrom
It might be possible to use ethabi for encoding, worth investigating if we want to support additional precompiles.
Resources