Handle Grandfathering of rate providers from pre-repo days

[Tritium-VLK]

Posting this for @Zen-Maxi who can provide more details in comments.

Complete list of current rate providers and their status in the ecosystem. Chainlink are separate but can likely be merged into the review section; this is a first pass. Legacy are unreviewed rate providers which are grandfathered in prior to our teams implementing reviews for rate contracts. Possible formatting clarifications are related to token -> rate provider address readability for ergonomic ease of use when querying. This will serve as a center of truth for future pool creations,****

[gosuto-inzasheru]

can you fix the lint please?

also there is a merge conflict

[mkflow27]

@Tritium-VLK What is the reason to add the "legacy" and "chainlink" entries? I don't recall what the FE team & API team have communicated about the expected layout. What do you think about leaving the format as it is and instead of grouping unreviewed/legacy ones onto the legacy section, add them according to the regular format and simply add the warning "unreviewed".

Similarly, the ChainlInk can be added into the same format and instead add the factory contract address into the "factory" entry?

I don't think we should break up the main structure of the file

Pinging @franzns. He created a branch with options here: https://github.com/balancer/code-review/tree/registry_schema_options

[franzns]

I believe the API will the the main (or even sole?) consumer of this, anybody else will get the data from the API (i.e. the pool composer). Also @mkflow27 should have a big say in this as he will be the one that actually needs to add entries.

I've created two options, a dict and a list:
Dict: https://github.com/balancer/code-review/blob/registry_schema_options/registry_schema1.json
List: https://github.com/balancer/code-review/blob/registry_schema_options/registry_schema2.json

I personally favor the list.

In general, everything is collapsed into one, no "legacy" or "chainlink" sections. What I would do, is add a "legacy" warning to the ones that have not (yet) received a review under the new process. For Chainlink, we can simply add it to the name (or also a warning, I'm open). For both, I wonder if it would make sense to have some kind of general markdowns that we can link to which would explain what the legacy or chainlink warnings mean?

[mkflow27]

Thanks @franzns For the format suggestions. Personally I would have stayed with the dict option, but am happy to switch.

I agree with how you think adding the "legacy" as a warning indicating that it does not have a review. The "Chainlink" would appear in the "name" already by the current review process as I usually put the contract name as the "name".

[Tritium-VLK]

I believe the API will the the main (or even sole?) consumer of this, anybody else will get the data from the API (i.e. the pool composer). Also @mkflow27 should have a big say in this as he will be the one that actually needs to add entries.

I've created two options, a dict and a list: Dict: https://github.com/balancer/code-review/blob/registry_schema_options/registry_schema1.json List: https://github.com/balancer/code-review/blob/registry_schema_options/registry_schema2.json

I personally favor the list.

In general, everything is collapsed into one, no "legacy" or "chainlink" sections. What I would do, is add a "legacy" warning to the ones that have not (yet) received a review under the new process. For Chainlink, we can simply add it to the name (or also a warning, I'm open). For both, I wonder if it would make sense to have some kind of general markdowns that we can link to which would explain what the legacy or chainlink warnings mean?

We have multisig reports that try to pull from the nearest source of truth and will likely pull this from github as well. I suppose we could use the API, but in that case we try to pull things from on-chain, as close to the source as possible.

I tend to prefer dict by chain/target to a list, but it is easy to build one from the other.

[Zen-Maxi]

I am indifferent to the format. The dict is more logical to me visually; but i do not spearhead either of our data consumption points so my opinion is mute honestly. If option two creates less technical hurdles overall maybe that is fine.

@gosuto-inzasheru merge conflict maybe due to stale data since the repo has been since updated by mk.

[franzns]

Seems people prefer the dict option. Let's do that.

How to handle "legacy" and "chainlink" rateproviders?

For legacy I propose to:

1. Add "legacy-review" to the warnings
2. define as "safe" under summary.
3. Add a general ./legacy-review.md file that explains a bit more about that

For chainlink I'm a bit unsure as I dont completely understand what has been done so far. But I guess each Factory and each rateprovider should have a review as well? (though they are pretty simple). Are there any Chainlink reviews as of now?

[mkflow27]

@franzns If a ChainLink RateProvider consumes a ChainLink price feed, no additional review is necessary. In this case, the Maxis? can simply open a PR with the standard format that mentions that is is a RP from the CL factory.

I think the next steps here would be to open a PR that:

1. adds the legacy rateProviders is the appropriate format which we agreed on during this conversation
2. Write up the legacy-review.md an include it in PR

The open question is:

• Are the Maxis (pinging @Tritium-VLK and @gosuto-inzasheru for visibility) willing to look up all ChainLink factory created rateProviders and add them to the registry and continue doing that?

I actually propose to split this into 2 PRs actually. One for the legacy RPs and one for the CL additions.

[Tritium-VLK]

@mkflow27 I think that's already done in the PR here by @Zen-Maxi right. Maybe he's the right one to feedback here, if he is willing to reformat his file and keep it up to date with new factory installs. Correct?

[Zen-Maxi]

What is our goal really? Mine was for one data source file to be where we can pull all rate providers from and the token they correspond to. This is easier for our automation and pool creation ux in the future. I have all the rate providers we have in prod here minus new ones since. It seems we want this formatted with a sub note for each "type" and there are 3 types, reviewed, chainlink, and legacy. Each of which has their own warning config.

[franzns]

@franzns If a ChainLink RateProvider consumes a ChainLink price feed, no additional review is necessary. In this case, the Maxis? can simply open a PR with the standard format that mentions that is is a RP from the CL factory.

Somebody still needs to check whether the correct price feed address has been added, right?

The open question is:

• Are the Maxis (pinging @Tritium-VLK and @gosuto-inzasheru for visibility) willing to look up all ChainLink factory created rateProviders and add them to the registry and continue doing that?

Yes, maxis will do that going forward. Current ones area already in @Zen-Maxi's PR

What is our goal really? Mine was for one data source file to be where we can pull all rate providers from and the token they correspond to. This is easier for our automation and pool creation ux in the future. I have all the rate providers we have in prod here minus new ones since. It seems we want this formatted with a sub note for each "type" and there are 3 types, reviewed, chainlink, and legacy. Each of which has their own warning config.

Agree, that's the goal and I think your PR already achieves this if collapse the legacy and chainlink "categories" into the global list and add a warning. One thing I want to stress again is that the API will be the major consumer, any UI flow will use the API to get the data from and not read this file. If you guys want to read the file for the msig operations, sure. But I'd even argue you should use the API as well.

So I think what is left is

1. Adjust @Zen-Maxi PR to "collapse" chainlink and legacy RPs to the list and add the warnings. @Zen-Maxi can you do that?
2. Write up a general legacy-review.md @mkflow27 can you do that?
3. Write up a general chainlink-review.md @mkflow27 can you do that?

[Zen-Maxi]

Please look at the last commit where i collapsed in legacy and chainlink rate providers. Only thing missing is a review md that i can include for the respective groups.

[mkflow27]

Done as per #63