pendulum-chain / pendulum-squids

The subsquid squids we use for Pendulum/Amplitude/Foucoco.
GNU General Public License v3.0
0 stars 0 forks source link

23 migrate to arrowsquid #28

Closed ebma closed 1 year ago

ebma commented 1 year ago

Closes #23.

For Reviewing

Keep in mind that everything under folder types as well as abi is automatically generated.

Typegen:

Since the last generation of types, subsquids appears to have changed how types for events, storage, etc are generated and used. The types for the 3 chains were regenerated as follows:

The types for nabla contract's ABI was generated by:

Changes in code:

Failing event:

NOTE: this event fails in the same way also with the previous implementation.

Only one event fails to be indexed by the processor in foucoco which is a state call to the method coverage() of the backstop pool contract at address 0x31e03c4b8ac908956b077124e9a0b93a18ae06de2ff62840d82e658258405c57 which produces a value that errors when attempts to decode the result to [u256, u256].

ebma commented 1 year ago

Great job @gianfra-t, it looks good overall. Just two concerns: If you try the following query on the v8 vs v9 foucoco squid:

query MyQuery {
  users {
    id
  }
  transfers(orderBy: id_ASC, limit: 10) {
    id
  }
}

You'll notice that the ID of the transfers changed (basically switching some parts) and there are more users in the v9 squid than before. Some users with hex ID were added. Do you know what the reason for these new users could be?

Besides that, I would suggest that we put the instructions for generating the abi files (which you nicely described in the description) into the README. We can even think of adding something to the commands.json file that automates this for all files contained in the nabla-abi directory. WDYT?

gianfra-t commented 1 year ago

Good finding! With respect to the user as hex, I would assume that is an error in the code, most likely a missing encode(user) call. I will try to find where is this missing call. Now that I think about it, this may be the cause for the extra users.

But regarding the different order in the ID of the transfer, I think this is how ArrowSquids processes the events now, since we do not modify the ID but save it as it arrives. In case this causes conflicts with existing consumers of the API we would need to manually switch the order probably.

Generating the types for the ABI with a command sounds good. Let me look into it and make a script that loops through the files on that directory.

gianfra-t commented 1 year ago

An update, indeed the extra users represented as hex were an issue with a missing encoding when storing. Now the query should return the same values (apart from the shifted id).

ebma commented 1 year ago

Can confirm that the queries now return the same users 👍 About the transfer ID's, we don't have to worry about that too much. If as you say we just use the ID the way it arrives, then let's keep it as is. We don't have existing services integrated that would break because of this change.

Regarding the abi generation, did you forget to push the generate-abi.js script? 😅

gianfra-t commented 1 year ago

Well apparently it was ignored by git and I didn't realize 😳. Now it is up. Keep in mind that when generating the ABI's some things will break. For instance, type string = string is declared and needs to be removed manually.

But yes in term of the ID take a look at this line for example. It is quite clear we use it "as-is" and we didn't change it during this migration. I could not found any counter example.

ebma commented 1 year ago

Nice, thanks for pointing this out, and thanks for all the great work with this.