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

Add "supports" to chains #86

Closed
wants to merge 4 commits into from
Closed

Add "supports" to chains #86

wants to merge 4 commits into from

Conversation

andreogle
Copy link
Member

@andreogle andreogle commented Sep 10, 2023

Changes

  1. Adds a required supports array to each chain. The possible values are any combination of these literals: ['API3Market', 'ChainAPI', 'dAPIs', 'OEVRelay']. Empty arrays are allowed too. I used this file as the reference for the actual values.

Addresses: #3

@andreogle andreogle added the enhancement New feature or request label Sep 10, 2023
@andreogle andreogle self-assigned this Sep 10, 2023
@bbenligiray
Copy link
Member

I have one doubt about this. Say we want to start supporting dAPIs with a new chain. We first need to update and publish @api3/chains, import that in @api3/airnode-protocol-v1 and deploy the contracts there. During the time between the two, @api3/chains will claim that the chain is supported even though it is not. I guess we should publish a release candidate or something instead for that?

@bbenligiray
Copy link
Member

For example, base, kava, linea, mantle, rsk support API3Market in https://github.com/api3dao/airnode-protocol-v1/blob/main/src/supported-chains.js#L3 though they are not live yet. What to do about this? (Also the current state is inconsistent, mantle supports Api3Market and linea does not, even though they will be added at the same time.)

src/types.ts Outdated
@@ -1,5 +1,7 @@
import { z } from 'zod';

export const SUPPORTS = ['API3Market', 'ChainAPI', 'dAPIs', 'OEVRelay'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would either do camel case (Api3Market, ChainApi, Dapis, OevRelay) or full names (API3 Market, ChainAPI, dAPIs, OEV Relay)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made these all fully lower case for now to avoid any kind of ambiguity or confusion. It seems the simplest to me (e.g. the example above is pascal case, not camel case 😬). What do you think?

@andreogle
Copy link
Member Author

andreogle commented Oct 13, 2023

We first need to update and publish @api3/chains, import that in @api3/airnode-protocol-v1 and deploy the contracts there. During the time between the two, @api3/chains will claim that the chain is supported even though it is not. I guess we should publish a release candidate or something instead for that?

Unfortunately I'm not really sure how to get around this if we want to make this package the source of truth for what is supported. I'm actually wondering if it's even the best way to go at all.

I think each project should be responsible for determining (and exposing if necessary) what it supports. Then we don't have this kind of chicken and egg problem.

The original issue was created to reduce confusion about what chains Api3ServerV1 is deployed on. I think a better approach is probably just to have the contract addresses listed in this repo and importing projects can figure out what they want to do with that information. i.e.

{
  "contracts": {
    "AirnodeRrpV0": null, // if not deployed yet
    "Api3ServerV1": "0x1234",
    "AccessControlRegistry": "0x9876",
    ...
  }
}

I realise that this is duplicating some of the references.json data though, but this repo seems like it's maybe the better place to store all information related to each chain.

What do you think?

@bbenligiray
Copy link
Member

The fundamental issue is

API3 provides services on independent sets of chains (which are all listed in @api3/chains). There is no central source of truth for what services we provide on each chain.

We're already doing it in a non-central way but that has two downsides:

  • Services typically don't have packages because they are ongoing operations and not code that needs to be distributed (ChainAPI, QRNG, Market, OEV Relay). I try to create packages anyway for this sort of things but even then, I didn't get around to implement the supports functionality specifically (@api3/contracts and @api3/airnode-protocol-v1 export references.json that the user should infer this information from, @nodary/utilities hasn't implemented it yet). Implementing this here, once and for all would be convenient.
  • If these all live in different packages, I'll have to keep track of these (as I'm currently doing at the moment, but in different ways in different places). The chain integration checklist is already too long and one of my main goals at the moment is to redesign processes to minimize it.

In short, I don't think that we need to export the contract addresses here. In most cases you would need to import the ABI along with the address, so it makes sense for these to be imported from the respective repos that deploy them. I think we actually need a supports feature and force everyone to have their implementations depend on it due to the fact that left on their own, people aren't converging to a solution. For example, try to tell what https://github.com/api3dao/api3-market does to add a new chain vs what https://github.com/api3dao/operations-database does vs what https://github.com/api3dao/data-feed-reader-example does vs what https://docs.api3.org/reference/qrng does. We have way too many of such services and this is all being maintained by a few people keeping mental notes at the moment.

@andreogle
Copy link
Member Author

I'm still a tiny bit skeptical about this but don't have a better solution at the moment so I've updated the PR and I think it should be ready to merge.

I guess we should publish a release candidate or something instead for that?

I guess so yeah. Or just a patch release

@bbenligiray
Copy link
Member

I'm also not sure, let me dwell on this a bit more

@bbenligiray bbenligiray self-requested a review October 30, 2023 12:01
@bbenligiray
Copy link
Member

bbenligiray commented Oct 30, 2023

I'm now thinking that this was a bad idea. You can close the PR if you still think that way. Specifically I think we should remove everything and only leave api3market, but @api3/dapi-management is already the perfect package to export that so even that is not needed.

@andreogle
Copy link
Member Author

Ok. Will re-open/re-implement if we want this again later

@andreogle andreogle closed this Oct 30, 2023
@andreogle andreogle deleted the feature/add-supports branch October 30, 2023 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants