neume-network / strategies

Indexing strategies for the neume network.
GNU General Public License v3.0
11 stars 7 forks source link

add generic call-tokenuri strategy #306

Closed il3ven closed 1 year ago

il3ven commented 2 years ago

This PR is part of the effort to improve strategy interface and increase ease of adding new contracts.

I recommend reviewing individual commits. Combining comments from individual commits here:

add an outputFilename option for strategies

Strategies can now optionally define outputFilename in crawlPath. For the following crawl path.

{
  name: "get-tokenuri",
  extractor: {
    args: [resolve(env.DATA_DIR, "soundxyz-call-tokenuri-transformation")],
    outputFilename: "soundxyz-get-tokenuri"
  },
  transformer: {},
}

The results for get-tokenuri will be written to soundxyz-get-tokenuri-extraction.

add a generic call-tokenuri strategy

Added a generic call-tokenuri strategy to remove the need for diffferent strategies such as soundxyz-call-tokenuri, zora-call-tokenuri, catalog-call-tokenuri etc.

The benefits include:

This new call-tokenuri strategy has been modified to not need logs-to-subgraph strategy. I have tested this strategy and the output files are the same as before.

Future scope of work:

TimDaub commented 1 year ago
  • Remove call-tokenuri strategy factory.

So I imagine that your process is as follows:

  1. get this PR merged
  2. then step by step remove call-tokenuri-factory implementations with more sophisticated crawlpath configurations, e.g. replacing the soundxyz-call-tokenuri extractor strategies

If so, I'm on-board with this process, I think it's good.

TimDaub commented 1 year ago

This new call-tokenuri strategy has been modified to not need logs-to-subgraph strategy.

really? How would we fill out track.erc721.createdAt (which is the track's block number at creation?). createdAt always has to be correctly populated as otherwise music os stops working (their "latest tracks" page). But maybe I'm misunderstanding something.

TimDaub commented 1 year ago

I'm gonna attempt to merge the following commits directly in main

TimDaub commented 1 year ago

I'm gonna attempt to merge the following commits directly in main

* [94a65e8](https://github.com/neume-network/strategies/commit/94a65e8b7d33bc45c1249744eb79b30b27b3bbf1)

* [a22ed5f](https://github.com/neume-network/strategies/commit/a22ed5f06387e28897009fd123bca3edc71dce11)

seemed to have worked, nice :)

TimDaub commented 1 year ago

created a separate issue in /schema: https://github.com/neume-network/schema/issues/50

TimDaub commented 1 year ago

man I love working with stacked commits. @il3ven highly recommended to check this concept out. I think google etc. all use this. They isolate commits by what they do roughly and so any commit can be individually applied to main and it makes integration into to main branch super smooth (like in what we just did with your two commits).

il3ven commented 1 year ago

So I imagine that your process is as follows:

get this PR merged then step by step remove call-tokenuri-factory implementations with more sophisticated crawlpath configurations, e.g. > > > replacing the soundxyz-call-tokenuri extractor strategies If so, I'm on-board with this process, I think it's good.

Exactly

really? How would we fill out track.erc721.createdAt (which is the track's block number at creation?). createdAt always has to be correctly populated as otherwise music os stops working (their "latest tracks" page). But maybe I'm misunderstanding something.

I meant that instead of using logs-to-subgraph I am directly using call-block-logs-transformation. We already have all the information we need in call-block-logs, don't we?

Sample output of call-block-logs-transformation ``` [ { "metadata": { "platform": { "name": "sound" } }, "log": { "address": "0x28b708ee88e608f3307a7d41b930312fc9c9e632", "topics": [ "0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef", "0x0000000000000000000000000000000000000000000000000000000000000000", "0x000000000000000000000000d5481575130a7decf4503c81a50152b3753031f9", "0x000000000000000000000000000000040000000000000000000000000000002c" ], "data": "0x", "blockNumber": "0xe88d6e", "timestamp": "0x0", "transactionHash": "0x46cb3cba91b478169df31184f45ebf3d1913335066414b76c645060fbd3edae8", "transactionIndex": "0x14b", "blockHash": "0x9affd9dfbad0300cda088fd525828d46571ea66987c520e97a66cf692c54a63f", "logIndex": "0xb7", "removed": false } } ] ``` We have `log.blockNumber` for `track.erc721.createdAt`.
TimDaub commented 1 year ago

I meant that instead of using logs-to-subgraph I am directly using call-block-logs-transformation. We already have all the information we need in call-block-logs, don't we?

ah ok I understood. Yeah I'm also in favor of crawling directly from call-block-logs

What's left for finalizing this PR? Can we merge it?

il3ven commented 1 year ago

What's left for finalizing this PR? Can we merge it?

Yes, we can merge. I will open new PRs to get closer to our goal of being able to incorporate new artists easily. Mainly the points outlined in the future scope of work above.

il3ven commented 1 year ago

I have pushed two fixes for fixing the tests. 70179f9f8675a1ddfee661ee06633ca9382b1a3c is to remove results and error from the new call-tokenuri because we have updated schema in main.

5c2462a808f4e00d7acb68a0ce7f393d20bd33a1 was to add a filter in ava to only run tests ending with _test.mjs. This is because in this PR I have added an extractor_snapshot.mjs and ava was picking it up. Because of no tests in it, ava was returning with exit code 1 even though all the other tests passed.

extractor_snapshot.mjs was added because the strategy accepted a function as an input and I can't define it in extractor_snapshot.json.

TimDaub commented 1 year ago

thanks :)