Skip to content
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 currency binding #899

Merged
merged 71 commits into from
Oct 17, 2024
Merged

feat: ERC20 currency binding #899

merged 71 commits into from
Oct 17, 2024

Conversation

mrq1911
Copy link
Member

@mrq1911 mrq1911 commented Sep 4, 2024

provides the ability for currencies pallet (and fungibles trait) to operate with the ERC20 contracts deployed in EVM
also allowed contracts can operate with substrate currencies through ERC20 precompiles

wip todo

  • fix unit tests mocks
  • merge integration tests
  • re-benchmark
  • runtime api's
  • bump versions

future todo

  • aave integration tests
  • reentrancy integration tests
  • locks and reserves support for erc20 currencies
  • support deposit & withdraw to holding account
  • helper to add erc20 currency easily to asset registry

Copy link

github-actions bot commented Sep 4, 2024

New crates:

  • pallet-currencies-rpc-runtime-api: v1.0.0

Crate versions that have been updated:

  • runtime-integration-tests: v1.23.7 -> v1.24.0
  • hydradx: v14.0.1 -> v14.0.2
  • pallet-asset-registry: v3.2.5 -> v3.3.0
  • pallet-claims: v3.4.10 -> v3.4.11
  • pallet-currencies: v2.1.2 -> v3.0.0
  • pallet-dca: v1.6.1 -> v1.6.2
  • pallet-duster: v3.2.6 -> v3.2.7
  • pallet-dynamic-evm-fee: v1.0.3 -> v1.0.4
  • pallet-evm-accounts: v1.1.3 -> v1.2.0
  • pallet-genesis-history: v2.1.3 -> v2.1.4
  • pallet-otc: v2.0.2 -> v2.0.3
  • pallet-otc-settlements: v1.0.4 -> v1.0.5
  • pallet-referrals: v1.2.6 -> v1.2.7
  • pallet-route-executor: v2.6.1 -> v2.6.2
  • pallet-transaction-multi-payment: v10.1.1 -> v10.1.2
  • pallet-xyk: v6.5.2 -> v6.5.3
  • primitives: v6.0.4 -> v6.1.0
  • hydradx-adapters: v1.3.6 -> v1.3.7
  • hydradx-runtime: v262.0.0 -> v264.0.0
  • module-evm-utility: v2.21.1 -> v2.21.2
  • hydradx-traits: v3.7.1 -> v3.7.2

Runtime version has been increased.

provenance,
)
match T::BoundErc20::contract_address(asset) {
Some(_) => DepositConsequence::UnknownAsset,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why there is unknown asset here ? as it seems to be clearly known. similar questions for other methods too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CannotCreate would be better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not sure what else to choose from WithdrawConsequences

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be resolved by just supporting deposits i guess

@@ -758,6 +761,24 @@ impl<T: Config> Create<Balance> for Pallet<T> {
}
}

impl<T> BoundErc20 for Pallet<T>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would not implement this for the pallet. I'd consider having something in our adapters that does this.

Copy link
Member Author

@mrq1911 mrq1911 Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i remember why is here ... as it completely relies on local functions - asset registry storages (asset type & location) and it is very tightly bound to it, i don't really see any advantage of moving it out now

Copy link
Member Author

@mrq1911 mrq1911 Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also i would like to add new extrinsic to asset registry to register these erc20 by specifying contract address (basically write version of this function)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for not having this in the pallet

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vgantchev reason?

@@ -197,7 +226,13 @@ where
if asset == T::GetNativeCurrencyId::get() {
<T::NativeCurrency as fungible::Mutate<T::AccountId>>::mint_into(who, amount.into()).into()
} else {
<T::MultiCurrency as fungibles::Mutate<T::AccountId>>::mint_into(asset.into(), who, amount.into()).into()
match T::BoundErc20::contract_address(asset) {
Some(_) => fail!(Error::<T>::NotSupported),
Copy link
Contributor

@enthusiastmartin enthusiastmartin Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not supported?

in the implementaiton of multicurrecny, you call deposit in case of deposit ( mint ).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you check implementation of Erc20Currency that yields unsupported as well, mby cleaner would be to call deposit implementation from multicurrency here as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more interesting would be to support deposit and withdraw by using some holding account (withdraw transfers to holding, deposit transfers from holding)

pallets/currencies/src/fungibles.rs Show resolved Hide resolved
pallets/currencies/src/lib.rs Show resolved Hide resolved
pallets/currencies/src/lib.rs Show resolved Hide resolved
# Conflicts:
#	Cargo.lock
#	integration-tests/Cargo.toml
#	pallets/route-executor/Cargo.toml
@mrq1911 mrq1911 marked this pull request as ready for review September 7, 2024 08:52
pallets/evm-accounts/src/lib.rs Show resolved Hide resolved
integration-tests/Cargo.toml Outdated Show resolved Hide resolved
pallets/asset-registry/src/tests/evm.rs Outdated Show resolved Hide resolved
pallets/asset-registry/src/tests/evm.rs Outdated Show resolved Hide resolved
pallets/evm-accounts/src/lib.rs Show resolved Hide resolved
pallets/evm-accounts/src/lib.rs Show resolved Hide resolved
runtime/hydradx/src/evm/erc20_currency.rs Show resolved Hide resolved
mrq1911 and others added 28 commits September 20, 2024 16:25
# Conflicts:
#	Cargo.lock
#	integration-tests/Cargo.toml
#	runtime/hydradx/Cargo.toml
#	runtime/hydradx/src/lib.rs
# Conflicts:
#	Cargo.lock
#	integration-tests/Cargo.toml
#	pallets/dca/Cargo.toml
#	pallets/liquidity-mining/Cargo.toml
#	pallets/route-executor/Cargo.toml
#	pallets/transaction-multi-payment/Cargo.toml
#	pallets/xyk-liquidity-mining/Cargo.toml
#	pallets/xyk/Cargo.toml
#	runtime/hydradx/Cargo.toml
#	traits/Cargo.toml
# Conflicts:
#	integration-tests/src/contracts.rs
…need to reinmplement the whole frontier runner and executer which comes with too much work
# Conflicts:
#	Cargo.lock
#	integration-tests/Cargo.toml
#	runtime/hydradx/Cargo.toml
#	runtime/hydradx/src/lib.rs
# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	integration-tests/Cargo.toml
#	node/Cargo.toml
#	pallets/asset-registry/Cargo.toml
#	pallets/claims/Cargo.toml
#	pallets/currencies/Cargo.toml
#	pallets/evm-accounts/Cargo.toml
#	pallets/otc-settlements/Cargo.toml
#	pallets/otc/Cargo.toml
#	primitives/Cargo.toml
#	runtime/hydradx/Cargo.toml
#	runtime/hydradx/src/evm/evm-utility/Cargo.toml
#	runtime/hydradx/src/lib.rs
#	runtime/hydradx/src/weights/pallet_currencies.rs
#	runtime/hydradx/src/weights/pallet_evm_accounts.rs
# Conflicts:
#	Cargo.lock
#	runtime/hydradx/Cargo.toml
#	runtime/hydradx/src/lib.rs
# Conflicts:
#	Cargo.toml
Copy link
Contributor

@jak-pan jak-pan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful

@mrq1911 mrq1911 merged commit 0ec0141 into master Oct 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants