CERT 7649 Allow dynamic price feed providers #20

nivcertora · 2024-11-28T10:25:20Z

https://certora.atlassian.net/browse/CERT-7649

PR Summary: Refactoring and Enhancements to Git Management and Price Feed Integration

This pull request introduces significant refactoring and enhancements to the Quorum project, focusing on improving the modularity, scalability, and maintainability of Git repository management and price feed integrations. Below are the key changes included in this PR:

1. Refactored GitManager

Enhanced Initialization:
- Modified the GitManager class to accept a ground_truth configuration (gt_config) directly during initialization.
- Benefit: Promotes decoupling by allowing GitManager to utilize pre-loaded configuration data, enhancing flexibility and testability.

2. Implemented Price Feed Providers

Introduced price_feed_utils.py:
- Defined PriceFeedProvider enum, PriceFeedData Pydantic model, and PriceFeedProviderBase abstract base class.
- Purpose: Establishes a clear interface and standardized data models for all price feed providers.
Updated Price Feed APIs:
- ChainLinkAPI & ChronicleAPI:
  - Now inherit from PriceFeedProviderBase.
  - Transitioned to a decorator-based Singleton pattern using the @singleton decorator, replacing metaclass-based Singleton implementations.
  - Implemented the _get_price_feeds_info method to adhere to the abstract base class requirements.
  - Benefit: Ensures consistency across different price feed providers and simplifies Singleton implementation.

3. Introduced PriceFeedCheck

New PriceFeedCheck Class:
- Replaces the deprecated FeedPriceCheck class.
- Functionality:
  - Verifies price feed addresses in source code against data from multiple providers (e.g., Chainlink, Chronicle).
  - Utilizes the Strategy pattern by accepting a list of PriceFeedProviderBase instances, allowing dynamic integration of various providers.
- Benefit: Enhances scalability by supporting multiple price feed providers and improving verification mechanisms.

4. Configuration Enhancements

Renamed Configuration File:
- Changed repos.json to ground_truth.json to better reflect its purpose.
- Updated all references in the codebase, documentation, and setup scripts accordingly.
Added config_loader.py:
- Centralizes the loading and validation of customer configurations from ground_truth.json.
- Features:
  - Validates supported price feed providers.
  - Maps provider names to their respective API instances (ChainLinkAPI, ChronicleAPI).
- Benefit: Streamlines configuration management and ensures consistent handling of customer-specific settings.

5. `.gitignore` Update

Ignored Cache Directories:
- Added **cache** to .gitignore to exclude all cache directories from version control.
- Benefit: Prevents accidental inclusion of cache data, maintaining a clean repository.

6. Documentation and Packaging Updates

Updated README.md:
- Reflected the renaming of repos.json to ground_truth.json.
- Provided updated examples and clarified the structure and purpose of configuration files.
Modified setup.py:
- Ensured that ground_truth.json is included in the package data for proper distribution.
- Benefit: Guarantees that necessary configuration files are available upon installation.

…m:Certora/Quorum into niv/CERT-7649-Support-Different-providers

liav-certora

Left some comments but overall looks good for what this PR aims to achieve.

liav-certora · 2024-12-01T10:17:26Z

Quorum/utils/config_loader.py

+    # Replace the provider names with the actual API objects
+    for i, provider in enumerate(providers):
+        if provider == PriceFeedProvider.CHAINLINK:
+            providers[i] = ChainLinkAPI()
+        elif provider == PriceFeedProvider.CHRONICLE:
+            providers[i] = ChronicleAPI()


I think saving some kind of mapping may be better but for now (small number of providers) this is also ok

liav-certora · 2024-12-01T10:27:15Z

Quorum/check_proposal.py

@@ -45,7 +47,7 @@ def load_config(config_path: str) -> dict[str, Any] | None:
        pp.pretty_print(f"Failed to parse given config file {config_path}:\n{e}", pp.Colors.FAILURE)


-def proposals_check(customer: str, chain_name: str, proposal_addresses: list[str]) -> None:
+def proposals_check(customer: str, chain_name: str, proposal_addresses: list[str], providers: list[PriceFeedProviderBase]) -> None:


Again, for now, it doesn't seem like that big of a deal but I don't like that we need to pass providers just for a single check out of all other checks. I think letting the PriceFeedCheck itself have access to the config would have been better. I can live with this for now but as we'll implement more and more checks, extending the parameters to this function might become really ugly.

Agree but its much better for testing to make it input instead of a dependancy

So i go with that solution

liav-certora · 2024-12-01T10:37:16Z

Quorum/apis/price_feeds/price_feed_utils.py

+        if cache_file.exists():
+            with open(cache_file, 'r') as file:
+                data: dict = json.load(file)
+            self.memory[chain] = {addr: PriceFeedData(**feed) for addr, feed in data.items()}


I may be missing something here but price feed data isn't updating? I mean I see you never update the cache only create it or read from it.

Create is the update, the same as it worked before

liav-certora · 2024-12-01T10:38:31Z

Quorum/ground_truth.json

Don't know why but I don't really like the name ground_truth but I also doesn't have a better name.

Renamed Configuration File:

Changed repos.json to ground_truth.json to better reflect its purpose.

CERT 7649 Allow dynamic price feed providers

162918e

nivcertora requested a review from a team November 28, 2024 10:25

nivcertora self-assigned this Nov 28, 2024

nivcertora and others added 6 commits November 28, 2024 12:26

Address comments

8552c77

Fix

7e938bb

New design + add cache to prevent api dealy

7de0a7a

Merge 7de0a7a into 23cef17

9ae90f5

Address comments

b6b0116

Auto change version.

87f3dd3

github-actions bot force-pushed the niv/CERT-7649-Support-Different-providers branch from 46925d2 to 87f3dd3 Compare November 28, 2024 16:12

nivcertora and others added 4 commits November 28, 2024 19:24

Update Readme

1441b95

Merge branch 'niv/CERT-7649-Support-Different-providers' of github.co…

9b39541

…m:Certora/Quorum into niv/CERT-7649-Support-Different-providers

Address comments

e7bd5f0

Merge e7bd5f0 into 23cef17

69acf55

github-actions bot force-pushed the niv/CERT-7649-Support-Different-providers branch from e7bd5f0 to e54fabe Compare November 28, 2024 17:26

Auto change version.

b4a2bbb

github-actions bot force-pushed the niv/CERT-7649-Support-Different-providers branch from e54fabe to b4a2bbb Compare November 28, 2024 17:27

liav-certora approved these changes Dec 1, 2024

View reviewed changes

nivcertora merged commit 5070b95 into main Dec 1, 2024

nivcertora deleted the niv/CERT-7649-Support-Different-providers branch December 1, 2024 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CERT 7649 Allow dynamic price feed providers #20

CERT 7649 Allow dynamic price feed providers #20

nivcertora commented Nov 28, 2024 •

edited

Loading

liav-certora left a comment

liav-certora Dec 1, 2024

nivcertora Dec 1, 2024

liav-certora Dec 1, 2024

nivcertora Dec 1, 2024

nivcertora Dec 1, 2024

liav-certora Dec 1, 2024

nivcertora Dec 1, 2024

liav-certora Dec 1, 2024

nivcertora Dec 1, 2024

CERT 7649 Allow dynamic price feed providers #20

CERT 7649 Allow dynamic price feed providers #20

Conversation

nivcertora commented Nov 28, 2024 • edited Loading

PR Summary: Refactoring and Enhancements to Git Management and Price Feed Integration

1. Refactored GitManager

2. Implemented Price Feed Providers

3. Introduced PriceFeedCheck

4. Configuration Enhancements

5. .gitignore Update

6. Documentation and Packaging Updates

liav-certora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nivcertora commented Nov 28, 2024 •

edited

Loading

5. `.gitignore` Update