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

44 migrate to new nabla contracts #52

Merged
merged 24 commits into from
May 29, 2024

Conversation

TorstenStueber
Copy link
Contributor

@TorstenStueber TorstenStueber commented Feb 19, 2024

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

src/mappings/token.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@gianfra-t gianfra-t left a 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).

src/mappings/nabla/handleEvent.ts Show resolved Hide resolved
src/mappings/nabla/routerEventHandler.ts Show resolved Hide resolved
src/mappings/nabla/swapPoolEventHandler.ts Show resolved Hide resolved
src/mappings/token.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
@TorstenStueber TorstenStueber marked this pull request as draft April 25, 2024 08:46
@ebma
Copy link
Member

ebma commented May 6, 2024

@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.

@TorstenStueber
Copy link
Contributor Author

@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.

@ebma
Copy link
Member

ebma commented May 13, 2024

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.

@TorstenStueber TorstenStueber marked this pull request as ready for review May 16, 2024 10:12
@TorstenStueber
Copy link
Contributor Author

@pendulum-chain/devs I addresses the comments from earlier reviews.

This PR can now be properly reviewed.

Copy link
Member

@ebma ebma left a 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 👍

src/mappings/nabla/creation.ts Show resolved Hide resolved
Copy link
Contributor

@bogdanS98 bogdanS98 left a 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 👍🏼

@ebma
Copy link
Member

ebma commented May 28, 2024

Let's merge @TorstenStueber?

Copy link
Contributor

@gianfra-t gianfra-t left a 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!

@TorstenStueber TorstenStueber merged commit b51940a into main May 29, 2024
@TorstenStueber TorstenStueber deleted the 44-migrate-to-new-nabla-contracts branch May 29, 2024 11:52
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.

Migrate to new Nabla contracts
4 participants