-
Notifications
You must be signed in to change notification settings - Fork 0
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
44 migrate to new nabla contracts #52
Conversation
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.
Looks very good! I really like the idea of comparing the hashes of the contract code to identify them.
I just left some comments (questions really).
@TorstenStueber I pushed a new commit that updates the ABI files of the Nabla contract to the ones compiled with a later version of Solang ie. v0.3.3 instead of v0.3.2. Please be aware that this now requires you to compile your Nabla contracts with the same version of Solang before using wasm-deploy to deploy them again. I only switched to v0.3.3. because it was the latest version available of Solang when I started testing everything one week ago. We can also roll it back if you want, but I guess it's good to stay up-to-date with the latest versions. |
@ebma we would either need to roll it back or deploy Nabla again on Foucoco. At least the current Nabla instance we use for all testing would not work with this version of the indexer anymore. |
Then let's deploy a new version of Nabla on Foucoco again or do you think this causes too much extra effort? AFAIK we'd would just need to change some hard-coded addresses in the portal and the rest can stay the same. |
This reverts commit 4ad7c45.
…m:pendulum-chain/pendulum-squids into 44-migrate-to-new-nabla-contracts
@pendulum-chain/devs I addresses the comments from earlier reviews. This PR can now be properly reviewed. |
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.
Looks good to me 👍
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.
Looks good to me too 👍🏼
Let's merge @TorstenStueber? |
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.
I checked the last commits since the previous review and looks all good also!
Closes #44
This PR implements the indexer changes for Nabla.
The changes are inspired by the according changes of the Ethereum based indexer for Nabla (see this PR: https://github.com/0xamberhq/subgraph-mumbai/pull/11).
However, there are quite a lot of necessary differences that mainly are caused by the fact that the Nabla contract addresses are hard coded in the subgraph indexer whereas we want to avoid hard coded address.
For that reason we need to reliably recognize what contracts are Nabla contracts. In the past we merely probed some methods to heuristically identify whether a contract event is caused by a specific contract type (router, swap pool, backstop pool, etc). However, this proved to be very unreliable and the indexer generated many errors.
Instead we decided to monitor
Contracts.Instantiated
events (see here) and then check the hash of the event causing the event and compare it with the contract metadata to correctly identify the type of contract.Other differences are to avoid some vulnerabilities that are in the subgraph indexer code:
Tests
I implemented a test suite in wasm deploy (see this PR: pendulum-chain/wasm-deploy#40).
This test suite required to run the wasm deploy test runner and foucoco-standalone locally. This also required to run the indexer (and the graphql server) locally. For that reason I added another configuration option
local
(otherwise subsquid will lookup the configuration for one of our chains in its archive and expects a specific genesis block – which will be different in foucoco-standalone). I am not sure whether this is the right approach