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

Remove adapter.ts duplication #293

Closed
wants to merge 141 commits into from

Conversation

ebarakos
Copy link
Contributor

@ebarakos ebarakos commented Feb 3, 2021

Removed adapter.ts from example, cryptoapis and amberdata as PoC.

Utilized endpoint Name, by making it a Names array. This might be useful on declaring new endpoint names while also keeping backwards naming compatibility.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2021

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

@ebarakos ebarakos force-pushed the refactor/remove-adapter-duplication branch from 59c6045 to 29cffc7 Compare February 3, 2021 16:03
@ebarakos ebarakos changed the title importing adapter.ts from bootstrap package Remove adapter.ts duplication Feb 3, 2021
@ebarakos ebarakos force-pushed the refactor/remove-adapter-duplication branch from d6060a0 to 4ad374c Compare February 3, 2021 16:34
justinkaseman and others added 14 commits February 3, 2021 12:44
…factor

coinmarketcap typescript refactor
* 1forge to TS

* Add Config type for recent change to generic types

* Utilize ExecuteWithConfig in 1forge

* Adds example README template & remove 'server' script

* Alphavantage adapter to TS

* Remove alphavantage-sdr folder

* Amberdata-gasprice to TS

* Anyblock-gasprice to TS

* Binance-DEX to TS

* Add note to binance-dex's differing use of API_ENDPOINT env var

* Brave New Coin to TS

* Bring Brave New Coin VWAP into Brave New Coin EA

* Coinapi to TS

* Coinbase to TS

* Coingecko to TS

* Move BNC helpers into BNC adapter

* Increase alphavantage test timeout

* Moves alphavantage APIkey param into endpoint - issue open for improvement

* Clean up leftover bravenewcoin-vwap traces

* Remove 1forge's conflicting and not useful endpoint param

* Requested changes: 1forge balance -> price, alphavantage example output

* Re-add server script & spread response data into endpoint responses

* Uses verbose env var to determine response data

* Gets API_KEY from makeConfig

* Add test dummy API_KEY

* Re-add server script to example
[develop] Preparing release v0.2.0-rc.1
* batch adapter TS refactor

* satisfy linter

* Uses config in generics, removes non-TS server script

* EA endpoints return their own response & use ExecuteWithConfig type

* Capitalize name property

* Use Config for baseURL and API_KEYS, re-add server script, use verbose env var for data response

Co-authored-by: Justin Kaseman <justinkaseman@live.com>
amberdata/src/index.ts Outdated Show resolved Hide resolved
@@ -3,7 +3,7 @@ import { Config } from '@chainlink/types'
import { Requester } from '@chainlink/external-adapter'
import { isChainType, isCoinType, BLOCKCHAINS } from '.'

export const Name = 'balance'
export const Names = ['balance']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really like this concept for backwards compatibility.

What it also allows for is if we embrace that "external-adapters" are a simple wrapper that map to a data providers API we could have the naming fit the API.

For example 1forge has an endpoint baseURL/convert that we use to get price data. So there we could set the name = ['price', 'convert']

Only tweak I would make is capitalizing it to denote a static property.

Suggested change
export const Names = ['balance']
export const NAME = ['balance']

Copy link
Contributor Author

@ebarakos ebarakos Feb 9, 2021

Choose a reason for hiding this comment

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

I am happy that you like it. I left the naming as I saw in some places, since we use NAME for the whole adapter package(btw, we can re-use a similar logic for routing between different adapter packages, e.g by using "source", as mentioned at the Composable Adapters document). Changing it to NAMES, to indicate array, until we think more regarding these.

@@ -1,7 +1,7 @@
import { makeExecute } from '../src/adapter'
import adapter from '../src/index'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could destructure this

Suggested change
import adapter from '../src/index'
import { makeExecute } from '../src/index'

Just a style preference thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some issues on the export = ES6 default exports of adapters' index.ts files, that's why it can't be used as the other imports. But it should be converted to this syntax once we resolve this.

bootstrap/src/adapter.ts Show resolved Hide resolved
bootstrap/src/adapter.ts Outdated Show resolved Hide resolved
import { DEFAULT_ENDPOINT } from '../config'

export const Name = 'bc_info'
export const Names = ['difficulty', 'height']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely important for backwards compatibility, but I wonder if in time these names should be housed in ea-legos since they are the specific data point to fetch.

In ea-legos it would map 'bc_info' => ['difficulty', 'height'].

So our Chainlink request would be for the data point block_height from cryptoapis which would then be transformed to call this API wrapper that knows how to make that HTTP request aka

{
  "endpoint": "bc_info",
  "path": "payload.headers"
}

I think this builds toward a future where data providers could just upload their whole API via an OpenAPI spec.

Don't think this is a right now thing, just thinking out loud.

typings/@chainlink/index.d.ts Outdated Show resolved Hide resolved
@justinkaseman
Copy link
Collaborator

Really like the direction, thanks for making a push toward this.

Left some thoughts, also curious to hear what @krebernisak thinks.

@ebarakos ebarakos force-pushed the refactor/remove-adapter-duplication branch from 4ad374c to 3974ec4 Compare February 8, 2021 11:28
@ebarakos ebarakos force-pushed the refactor/remove-adapter-duplication branch 2 times, most recently from d9c5182 to b88d7eb Compare February 24, 2021 08:47
@ebarakos ebarakos linked an issue Feb 24, 2021 that may be closed by this pull request
@ebarakos ebarakos force-pushed the refactor/remove-adapter-duplication branch 4 times, most recently from 657f99d to 5f49c2a Compare February 26, 2021 13:08
@HenryNguyen5
Copy link
Collaborator

What's the status on this PR @ebarakos @justinkaseman?

@justinkaseman
Copy link
Collaborator

Closing as these changes have become obsolete.

@austinborn austinborn deleted the refactor/remove-adapter-duplication branch November 4, 2021 17:51
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.

Improve how adapters structure and map internal API calls