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

Use runtime managed ERC20 tokens in tests #26

Closed
TorstenStueber opened this issue Sep 25, 2023 · 13 comments · Fixed by #27
Closed

Use runtime managed ERC20 tokens in tests #26

TorstenStueber opened this issue Sep 25, 2023 · 13 comments · Fixed by #27
Assignees

Comments

@TorstenStueber
Copy link
Contributor

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 TorstenStueber changed the title Use runtime manages ERC20 tokens in tests Use runtime managed ERC20 tokens in tests Sep 25, 2023
@TorstenStueber
Copy link
Contributor Author

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

@TorstenStueber
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

ebma commented Sep 28, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor

gianfra-t commented Dec 14, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Closing this as it has already been part of #23

@ebma
Copy link
Member

ebma commented Dec 20, 2023

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
Copy link
Contributor Author

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
Copy link
Member

ebma commented Jan 22, 2024

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
Copy link
Contributor Author

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

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 a pull request may close this issue.

3 participants