Use runtime managed ERC20 tokens in tests

TorstenStueber commented 1 year ago

The Nabla test use a MockERC20 contract. These contracts are an extension of the default open zeppelin ERC20 contracts with additional mint and burn functions.

These contracts should be replaced with the ERC20Wrapper contracts defined in the pendulum-ink-wrapper. However, this contract does not have mint and burn functions and the chain extensions in the foucoco-standalone chain doesn't provide this functionality either.

TODO

add mint and burn functions to the chain extension of the foucoco-standalone chain
add a new TestableERC20Wrapper contract to the pendulum-ink-wrapper repository that has burn and mint functions and that uses the chain extensions
replace the MockERC20 definition in nabla/config.json by referring to the TestableERC20Wrapper contract

TorstenStueber commented 1 year ago

Hey team! Please add your planning poker estimate with Zenhub @gianfra-t @b-yap @ebma

TorstenStueber commented 1 year ago

@pendulum-chain/product The purpose of this ticket is to change the Nabla tests to actually tokens managed in our runtime (which is our use case) opposed to assets managed in ERC20 contracts (which is the default in the Nabla tests).

This has high priority as this is required for successfully testing Nabla.

gianfra-t commented 1 year ago

One question to clarify, when deploying multiple instances of ERC20Wrapper that have the same token index, what stops these contracts to have the ability to also mint and burn ? Should we also make a modification to only allow one contract per associated token?

TorstenStueber commented 1 year ago

That is a good point and indeed there is no one stopping you to deploy wrapper contracts that point to the same token.

This is potentially confusing but no logical insecurity. The copy wrapper contract can do exactly what the original can do and any user could call into either one if they wanted to.

ebma commented 1 year ago

add mint and burn functions to the chain extension of the foucoco-standalone chain

What's our plan with the foucoco-standalone project? Is it just a temporary solution or are we planning to keep it? I'm asking because eventually we'd like to extract Pendulum's chain extension to an extra crate (see this issue), which could then also be re-used in the foucoco-standalone chain. It does not make sense to have the mint and burn functions in our production chain extension, so it makes sense to only add it to the foucoco-standalone chain extension. But if we are planning to keep the foucoco-standalone project in the future, we should consider exploring more re-usable options.

TorstenStueber commented 10 months ago

@ebma Yes, we should use the common chain extension here plus the additional mint and burn features.

I would rather not have to keep Foucoco standalone around and rather use our normal Foucoco. However,

I did not find a solution to execute tests quickly on Foucoco: Chopsticks did not work at that time – but this is now fixed, however, it seems to still be quite slow compared to manual-seal and the tests submit a ton of transactions, particularly fuzz tests
We need some extra functionality that Foucoco can not deliver, like fast forwarding blocks in tests and the mint and burn feature

Do you see a more elegant solution how we can have this "testable" version of Foucoco without needing to maintain this extra repo?

gianfra-t commented 10 months ago

The main difference is the import of pallet-contracts = { git = "https://github.com/pendulum-chain/substrate", branch = "1-add-advance-block-feature-for-pallet-contracts-1" } right? Perhaps we could add feature flags to the main foucoco repo and conditionally use that pallet whenever we want to test. Also do the same with the mint ant burn extension.

TorstenStueber commented 10 months ago

No, the main difference is that this uses manual seal, that means that blocks are immediately produced when a transaction is submitted. Otherwise testing would take a long time.

I don't know how to make manual seal work for Foucoco as this would need to happen in sync with the relay chain, i.e., we would not to run a local relay and parachain and both need to use manual seal.

Could be a research task of course.

TorstenStueber commented 10 months ago

Closing this as it has already been part of https://github.com/pendulum-chain/wasm-deploy/pull/23

ebma commented 9 months ago

About the manual-seal, we can add this to all our runtimes (or some of them) and enable it with a conditional flag. For example interlay is doing this, you can find the related code parts for manual-seal here and an open PR for allowing both manual or instant seal here. I am not sure what we can do about the fast-forwarding of blocks in the tests though. Maybe we should create a follow-up research ticket after all.

TorstenStueber commented 9 months ago

Nice find with the instant seal feature. Would make a lot of sense to add this. Do you know how that works with the relay chain?

For the skip block feature: one option would be to add use the altered pallet-contracts directly in Foucoco or use a feature flag. Nevertheless, I added this research ticket: https://github.com/pendulum-chain/tasks/issues/190

ebma commented 8 months ago

No, unfortunately I don't really. I assume that they only use it for their integration tests. While in our Spacewalk integration tests, we use a custom standalone chain that we configured to be usable with manual seal, I assume for their integration tests they launch a custom mock network and connect their runtimes to that with instant seal enabled. With the advantage of being able to use and test their production runtime instead of a custom one.

TorstenStueber commented 8 months ago

It's not super urgent as Foucoco-standalone is good enough for our current needs but in the future it is worthwhile to use this solution to avoid maintenance cost. I created this research ticket: https://github.com/pendulum-chain/tasks/issues/235

pendulum-chain / wasm-deploy

Use runtime managed ERC20 tokens in tests #26

TODO