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

feat: add Alpha Vantage apdater + and support for API keys #182

Merged
merged 18 commits into from
Sep 6, 2023

Conversation

nvtaveras
Copy link
Collaborator

@nvtaveras nvtaveras commented Aug 3, 2023

Description

This adds a first FX provider adapter (Alpha Vantage) which we will use for deploying the first XOF oracles on testnets. In addition to the adapter the client is also extended to support a new API_KEYS env variable in the format <exchange-identifier-1>:<api-key-1>,<exchange-identifier-2>:<api-key-2>, which is parsed and passed onto their respective adapters.

Other FX adapters will follow later on once we have more clarity on the final set of price sources.

Other changes

  • A bunch of linting changes done by prettier
  • Updated the bitstamp certificate as I found out it was expired while testing (we currently don't use it but will soon for EUROC/EUR rates)

Tested

  • Wrote unit tests for the new adapter
  • Ran the client locally using the new API keys env var

Related issues

Copy link
Collaborator

@philbow61 philbow61 left a comment

Choose a reason for hiding this comment

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

LGTM just some nits

src/utils.ts Outdated Show resolved Hide resolved
src/data_aggregator.ts Outdated Show resolved Hide resolved
src/exchange_adapters/alphavantage.ts Show resolved Hide resolved
src/exchange_adapters/alphavantage.ts Outdated Show resolved Hide resolved
src/envvar_utils.ts Outdated Show resolved Hide resolved
Comment on lines 89 to 90
baseVolume: new BigNumber(1),
quoteVolume: new BigNumber(1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

How difficult would it be to have volume be optional here, and then assigning "fake" volume, somewhere else in the chain in a way that it won't get fucked if we mix "fake volume" exchange with "real volume" exchanges.

Alternatively, could we add another field here, which is something like "volumeMissing: true" and set it to false in the priceObjectMetadata in Base, and then when aggregating throw an error unless either all of them are volumeMissing: true or false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated this and left a comment here outlining the approach I took

src/exchange_adapters/alphavantage.ts Outdated Show resolved Hide resolved
src/exchange_adapters/alphavantage.ts Outdated Show resolved Hide resolved
@nvtaveras
Copy link
Collaborator Author

Updated with a bunch of changes since the last review, the most important one being 628ca4e to add an ignoreVolume field to the price sources configuration. The change it's backwards compatible and the value will default to false when not passed in the configurations, so we don't need to update all the existing charts to add the extra field.

The idea is that we would set it for configurations that involve an FX rate along the way, for example, CELOXOF:

        {exchange: 'BINANCE', symbol: 'CELOUSDT', toInvert: false},
        {exchange: 'BINANCE', symbol: 'EURUSDT', toInvert: true},
        {exchange: 'ALPHAVANTAGE', symbol: 'EURXOF', toInvert: false, ignoreVolume: true}

while configurations that are only composed of FX rates (for example, EURXOF) don't need to set anything, as the default volume of 1 on FX adapters will ensure that they are all weighted equally.

The risk is that we could miss setting the property in a place where is actually necessary and end up "under-weighting" a price source, but imho this can just be part of the price source review process, similarly to how we need to ensure that the toInvert property is always correctly set.

@nvtaveras nvtaveras merged commit 8b54705 into main Sep 6, 2023
4 checks passed
@nvtaveras nvtaveras deleted the feat/alphaVantageAdapter branch September 6, 2023 12:11
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