Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

fix(middleware): oracles, tests #1944

Merged
merged 9 commits into from
Dec 18, 2022
Merged

Conversation

DaniPopes
Copy link
Collaborator

@DaniPopes DaniPopes commented Dec 16, 2022

Motivation

Old / non functioning API URLs

Solution

  • Replace gas_now url (old redirected to new)
  • deprecate eth_gas_station and replace it in docs and tests with gas_now
  • temporarily ignore etherchain tests which is failing in builds due to 404
  • clean up parsing and gas calculation since every oracle was different
  • add some Default impls and other derives
  • export internal Response structs which were exposed in the query functions but were not exported at mod level
  • add common Result type alias for gas_oracle
  • add missing tests for oracles
  • big tests diffs are just removing indentation from removing unnecessary mod tests
  • add missing fields in gas_now Response
  • make file structure consistent with each other (imports -> definitions -> [trait impls, impls])

minor breaking changes:

  • make blocknative API key optional, which also makes the estimatedBaseFees response field optional since I think a key is required for that
  • change some base oracles' methods from request to query for consistency
  • change all oracle query methods to return just the Response struct and move parsing in other functions

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • Breaking changes

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

tysm!

It looks like this includes a lot of valuable but separate contributions, including some breaking changes I think.

I'd rather merge this in separate PRs, so it would be easier to merge this step by step.

Could you please split this as you see fit?

@DaniPopes
Copy link
Collaborator Author

What do you mean by easier to merge? I think a lot of these changes will overlap with each other which would have the opposite effect - I don't think there's much point in splitting this, it's mostly just making the individual oracles and file structure consistent + fixing a few bugs and adding a few tests

I finished listing all changes and breaking changes

@mattsse
Copy link
Collaborator

mattsse commented Dec 17, 2022

What do you mean by easier to merge? I think a lot of these changes will overlap with each other which would have the opposite effect - I don't think there's much point in splitting this

this is good to know.

I just wanted to check this, before looking at it more closely because it looks like it has a lot of valuable fixes.

use crate::gas_oracle::{GasCategory, GasOracle, GasOracleError, GWEI_TO_WEI};

const ETHERCHAIN_URL: &str = "https://www.etherchain.org/api/gasPriceOracle";
const URL: &str = "https://www.etherchain.org/api/gasPriceOracle";
Copy link
Owner

Choose a reason for hiding this comment

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

@gakonst gakonst merged commit bb4af1c into gakonst:master Dec 18, 2022
@mattsse mattsse mentioned this pull request Dec 30, 2022
4 tasks
@DaniPopes DaniPopes deleted the fix/gas-oracles branch January 27, 2023 17:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants