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

[VEN-1895] Add Arbitrum sequencer downtime validation for Chainlink Oracle #128

Merged
merged 5 commits into from
Mar 22, 2024

Conversation

0xlucian
Copy link
Contributor

Description

This PR adds Chainlink Oracle support for Arbitrum.

Resolves #VEN-1895

Checklist

  • I have updated the documentation to account for the changes in the code.
  • If I added new functionality, I added tests covering it.
  • If I fixed a bug, I added a test preventing this bug from silently reappearing again.
  • My contribution follows Venus contribution guidelines.

@web3rover
Copy link
Contributor

web3rover commented Dec 11, 2023

@0xlucian Instead of creating a separate contract I think we can add a sequencerUptimeFeed immutable address. If it's not zero then verify the sequencer uptime. I think this way we don't need to maintain multiple chainlink oracle contracts.

Also sequencer is used in OP, ARB and Base networks. So if we decide to use a separate contract, then we should rename the contract from ArbiChainlinkOracle to SequencerChainlinkOracle

@chechu
Copy link
Member

chechu commented Dec 11, 2023

@0xlucian Instead of creating a separate contract I think we can add a sequencerUptimeFeed immutable address. If it's not zero then verify the sequencer uptime. I think this way we don't need to maintain multiple chainlink oracle contracts.

Also sequencer is used in OP, ARB and Base networks. So if we decide to use a separate contract, then we should rename the contract from ArbiChainlinkOracle to SequencerChainlinkOracle

I would rename the contract to SequencerChainlinkOracle. I would personally keep it in its own contract, to avoid deploying code to networks that we won't use it

@0xlucian
Copy link
Contributor Author

0xlucian commented Dec 11, 2023

@chechu @Narayanprusty I think that having different oracle that extends this functionality makes more sense.. On one side it will save us gas on Ethereum (because we will not have this logic at all) and on the other side I think having non-zero address logic is kinda prone to more errors.
Maybe I can just rename the oracle for L2ChainlinkOracle or SequencerChainlinkOracle as proposed by @chechu . What do you think?

@web3rover
Copy link
Contributor

I prefer SequencerChainlinkOracle as all L2s don't use a sequencer or need sequencer uptime validation.

@0xlucian
Copy link
Contributor Author

@chechu @Narayanprusty ArbiChainlinkOracle has been renamed to SequencerChainlinkOracle. Let me know if I can merge this PR?

@chechu chechu added this to the 024 milestone Feb 7, 2024
Copy link
Collaborator

@kkirka kkirka left a comment

Choose a reason for hiding this comment

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

LGTM overall

deploy/1-deploy-oracles.ts Show resolved Hide resolved
Copy link

Code Coverage

Package Line Rate Branch Rate Health
contracts 85% 80%
contracts.interfaces 28% 21%
contracts.oracles 96% 83%
Summary 86% (233 / 270) 77% (132 / 172)

@0xlucian 0xlucian merged commit d287c6e into develop Mar 22, 2024
4 checks passed
@chechu chechu mentioned this pull request Apr 10, 2024
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.

6 participants