feat(indexer): index all L1 & L2 contract events #5968
Conversation
|
✅ Deploy Preview for opstack-docs canceled.
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #5968 +/- ##
===========================================
- Coverage 42.95% 40.57% -2.39%
===========================================
Files 478 320 -158
Lines 30783 26115 -4668
Branches 877 0 -877
===========================================
- Hits 13223 10595 -2628
+ Misses 16533 14544 -1989
+ Partials 1027 976 -51
Flags with carried forward coverage won't be shown. Click here to find out more. |
| L1CrossDomainMessenger: common.HexToAddress("0x6900000000000000000000000000000000000002"), | ||
| L1StandardBridge: common.HexToAddress("0x6900000000000000000000000000000000000003"), | ||
| L1ERC721Bridge: common.HexToAddress("0x6900000000000000000000000000000000000004"), | ||
| } |
There was a problem hiding this comment.
Specifying contract addresses like this is a mistake Optimism consistently makes imo. doesn't have to be this pr but we really need to make it a point to try to use deployment artifacts as a single source of truth
There was a problem hiding this comment.
might go to client office hours to ask about this. Easiest solution imo is to have a go package which is generated from the artifacts
There was a problem hiding this comment.
There might actually already exist a solution like getContractArftifacts in js that I'm just not aware of as well
There was a problem hiding this comment.
also one more comment about this. These are actually coming from the config.toml file. It has a section in it called [chain] which would be where these are configured. I'm including a property though called preset that allows you to just use known chain presets for known chains like OP and base without having to specify everything though
There was a problem hiding this comment.
I'm all in for having presets that can be selected. Kinda left that open ended on the configuration side of things
The reason its important for the processor to not bias towards preset it so that it can work with any OP chain
There was a problem hiding this comment.
essentially nothing about the app other than the config should be aware of contract addresses and other configurable things
There was a problem hiding this comment.
gotcha i see what you mean. On the same page that the config the sole entry point for how these addresses are generated.
We definitely don't have a good approach to this now. I can also smell future complexity once we go through additional contract upgrades where the ABI has to be versioned
There was a problem hiding this comment.
Yup that's one reason I really want the build pipeline between contract artifacts and services like this to be as tight as possible. If it's tight, we avoid versioning issues because the contracts upgrade and the indexer upgrade are forced to stay in sync
There was a problem hiding this comment.
If it's tight, we avoid versioning issues because the contracts upgrade and the indexer upgrade are forced to stay in sync
I don't think this enforcement would be desired for the indexer. For example, if someone is resyncing the indexer from genesis, using the latest version isn't actually what you want. The lifecycle of an op-chain will consist of many versions/upgrades
There was a problem hiding this comment.
resolving this to merge PR but good conversation to follow up on
Sounds, lets merge this and continue iterating |
|
This PR has been added to the merge queue, and will be merged soon. |
|
Hey @hamdiallam, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by commenting with |
When starting the processor, the optimism contracts should be supplied. We watch these contracts and index as batches are processed.
Aside
Followup