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

CERT 7009 Add Chronicle Price Feed #16

Merged
merged 18 commits into from
Nov 5, 2024

Conversation

nivcertora
Copy link
Collaborator

@nivcertora nivcertora commented Nov 5, 2024

📢 PR Summary: Integrate ChronicleAPI and Enhance Feed Price Verification

🔧 Key Changes:

  1. API Module Restructuring:
    • Added Quorum/apis/price_feeds/__init__.py:
      • Imported and exposed ChainLinkAPI and ChronicleAPI for centralized access.
      • Defined __all__ = [ChainLinkAPI, ChronicleAPI] for clear API exposure.
    • Moved ChainLinkAPI:
      • Renamed and relocated Quorum/apis/chainlink_api.py to Quorum/apis/price_feeds/chainlink_api.py for better organization within the price_feeds package.
  • Moved git_api:
    • relocated Quorum/git_api to Quorum/apis/git_api for better organization.
  • Added ChronicleAPI:
    • Introduced Quorum/apis/price_feeds/chronicle_api.py to interact with the Chronicle data feed API.
    • Implemented ChronicleAPI as a singleton to efficiently manage API requests and cache price feed data.
  1. Feed Price Verification Enhancements:

    • Updated Quorum/checks/feed_price.py:
      • Imported ChainLinkAPI and ChronicleAPI: Adjusted import paths to the new price_feeds package.
      • Initialized Both APIs:
        • chainlink_api = ChainLinkAPI()
        • chronicle_api = ChronicleAPI()
      • Aggregated Price Feeds from Multiple Sources:
        • Fetched and stored price feeds from both Chainlink and Chronicle APIs into separate dictionaries for comprehensive verification.
      • Enhanced Verification Logic:
        • Extended verification to check price feed addresses against both Chainlink and Chronicle data sources.
        • Categorized addresses into verified and reported accordingly, providing more robust price feed validation.
  2. Utility Updates:

    • Updated Quorum/utils/chain_enum.py:
      • Renamed Blockchain Identifiers for Clarity and support chronicle name convention:
        • SCRSCROLL
        • ZKZKSYNC

✅ Benefits:

  • Comprehensive Feed Verification: Integrating ChronicleAPI alongside ChainLinkAPI allows the tool to verify price feeds against multiple authoritative sources, enhancing reliability.
  • Improved Code Organization: Structuring APIs within the price_feeds package promotes better maintainability and scalability.
  • Enhanced Verification Accuracy: Dual-source verification reduces the risk of relying on a single data provider, ensuring more accurate and trustworthy price feed checks.

@nivcertora nivcertora requested a review from a team November 5, 2024 09:04
@nivcertora nivcertora self-assigned this Nov 5, 2024
def __init__(self):
self.session = requests.Session()
self.memory = defaultdict(list)
self.info_url = "https://chroniclelabs.org/api/median/info/{}/{}/?testnet=false"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think either define this url in get_price_feeds_info or define the url from __process_pairs here. It looks funky that you treat urls differently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

price_feeds = self.api.get_price_feeds_info(self.chain)
self.price_feeds_dict = {feed.contractAddress: feed for feed in price_feeds}
self.price_feeds_dict.update({feed.proxyAddress: feed for feed in price_feeds if feed.proxyAddress})
price_feeds = self.chainlink_api.get_price_feeds_info(self.chain)
Copy link
Contributor

Choose a reason for hiding this comment

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

chainlink_price_feeds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks

Comment on lines 23 to 25
pairs = requests.get(
"https://chroniclelabs.org/api/pairs?testnet=false"
).json()
Copy link
Contributor

Choose a reason for hiding this comment

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

Any retries or raise_for_status needed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks added raise_for_status

Copy link
Contributor

@yoav-el-certora yoav-el-certora left a comment

Choose a reason for hiding this comment

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

Small comment for improvement but looks good

@nivcertora nivcertora merged commit 7f618a1 into main Nov 5, 2024
@nivcertora nivcertora deleted the niv/CERT-7009-Add-Chronicle-Price-Feed branch November 5, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants