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

[Feature] Crypto volatility index adapter (by COTI) #138

Merged

Conversation

yonine
Copy link
Contributor

@yonine yonine commented Nov 8, 2020

Description

An external adapter for calculating the Crypto volatility index (CVX)

Copy link
Contributor

@boxhock boxhock left a comment

Choose a reason for hiding this comment

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

Thanks for making this PR! Please see the comments that I left. I also see that the adapter is written in JS, but follows some of the structure we use for TS adapters.

We are working on moving more and more over to TS. Would you be able to rewrite this to TS, following the structure in the example adapter? It shouldn't be that big of a difference, just adding some types and replacing callbacks with async/await.

crypto-volatility-index/src/cryptoVolatilityIndex.js Outdated Show resolved Hide resolved
crypto-volatility-index/adapter.js Outdated Show resolved Hide resolved
crypto-volatility-index/src/cryptoVolatilityIndex.js Outdated Show resolved Hide resolved
crypto-volatility-index/src/derivativesDataProvider.js Outdated Show resolved Hide resolved
crypto-volatility-index/test/adapter_test.js Outdated Show resolved Hide resolved
crypto-volatility-index/package-lock.json Outdated Show resolved Hide resolved
crypto-volatility-index/adapter.js Outdated Show resolved Hide resolved
@yonine
Copy link
Contributor Author

yonine commented Nov 25, 2020

updated the adapter to TS as requested

Copy link
Contributor

@boxhock boxhock left a comment

Choose a reason for hiding this comment

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

There seems to be a few linter checks that are failing. We'll need that sorted before merging as well.

crypto-volatility-index/package.json Outdated Show resolved Hide resolved
crypto-volatility-index/package.json Outdated Show resolved Hide resolved
@krebernisak krebernisak changed the base branch from master to develop December 8, 2020 17:30
@yonine yonine force-pushed the feature/crypto-volatility-index branch 2 times, most recently from f8f3baf to 9515a4c Compare December 25, 2020 01:04
@yonine yonine force-pushed the feature/crypto-volatility-index branch 2 times, most recently from 18abda3 to b78b1f7 Compare January 12, 2021 22:03
Copy link
Contributor

@boxhock boxhock left a comment

Choose a reason for hiding this comment

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

Was able to run this adapter successfully on a Kovan feed! Just a couple comments below.

Copy link
Contributor

@boxhock boxhock left a comment

Choose a reason for hiding this comment

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

Just a few things that came to my attention when running it again.

typings/reference-data-reader/index.d.ts Outdated Show resolved Hide resolved
helpers/reference-data-reader/src/index.ts Outdated Show resolved Hide resolved
@yonine yonine force-pushed the feature/crypto-volatility-index branch from 7d34cbc to 50f89a5 Compare January 14, 2021 21:12
@boxhock boxhock merged commit 34582a5 into smartcontractkit:develop Jan 14, 2021
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.

2 participants