Add block height in relevant adapters #215

ebarakos · 2021-01-08T12:32:43Z

No description provided.

github-actions · 2021-01-08T12:33:33Z

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

ebarakos · 2021-01-08T12:52:06Z

Currently, the common interface between all 4(3 APIs + json-rpc) adapters that return bitcoin info is: { "blockchain": "BTC", q: "difficulty"} & { "blockchain": "BTC", q: "height"}.

I didn't add many validations on q, so it can potentially support other parameters as well.
Tried to make it backwards compatible, in case previous adapter params are already used by node operators. (in cryptoapis you can use endpoint instead of q and q param defaults to difficulty in other adapters).

We can improve the uniform interface/naming for blockchain info/stats, either now or in the future and choose a convention either a) created by us b) identical to the getblockchainfo params naming c) one of external APIs, like cryptoapis

krebernisak

Thanks, @ebarakos!

I tried to leave some comments and ideas, but we should probably also sync offline on this.

In general:

Please use the existing endpoint input param instead of a new one q (take a look at existing adapters using endpoint on input)
Don't add custom logic to our general json-rpc adapter, but you can make a new bitcoin-json-rpc adapter, that wraps/uses the underlying json-rpc but follows our standard API.
In the future we should rename our endpoint input param as dataPoint and codify all data point types that we support in TS + docs

blockchair/adapter.js

cryptoapis/adapter.js

json-rpc/src/adapter.ts

cryptoapis/adapter.js

krebernisak · 2021-01-08T15:18:51Z

blockchair/adapter.js

 const execute = (input, callback) => {
  const validator = new Validator(input, customParams)
  if (validator.error) return callback(validator.error.statusCode, validator.errored)

  const jobRunID = validator.validated.id
  let blockchain = validator.validated.data.blockchain
+  let q = validator.validated.data.q || 'difficulty'


We currently use an endpoint input param to differentiate between different data points we want to fetch from the external data provider.

I would suggest we don't introduce new behavior to the adapter API at this point but continue using what we have now.

Take a look at how we handle endpoint queries in different providers, and please try to mimic the interface and behavior here.

In the future, we are looking to streamline the adapter API. The first thing we should do is list the data points we support and codify them. Here you are adding support for a new data point, let's call it BlockNumber. All of our adapters that know how to answer requests for that specific data point should follow the same API (input/output).

In this context, it may make sense to wrap the json-rpc adapter into bitcoin-json-rpc adapter that will then expose this specific data point (and others).

krebernisak · 2021-01-08T15:26:31Z

cryptoapis/adapter.js

  network: false,
 }

-const difficulty = (jobRunID, input, callback) => {
-  const validator = new Validator(input, difficultyParams)
+const latestBlock = (jobRunID, input, callback) => {


I like how you renamed this as latestBlock, as this actually is the latestBlock endpoint from the cryptoapis data provider. We also call our input param endpoint, but IMHO this is the wrong name for this input param.

What our input param should be called is a dataPoint, which we then map to 1 (or more) underlying API endpoints from which we fetch data.

Here the cryptoapis adapter supports two data points (we call them endpoint) and for both of those (difficulty|height) we fetch data from the same underlying HTTP endpoint (latestBlock).

ebarakos · 2021-01-08T15:58:08Z

Thank you @krebernisak for your helpful review.

I will revert q to endpoint, along with the other changes, and we can tackle this in the future, across the whole codebase, as you mentioned.

ebarakos · 2021-01-11T13:18:43Z

Created bitcoin-json-rpc as composite adapter. It can be used with the same parameters as json-rpc, but also adheres to the { "blockchain": "BTC", endpoint: "difficulty"} & { "blockchain": "BTC", endpoint: "height"} convention of other relevant adapters (blockchain param can be omitted).

composite/bitcoin-json-rpc/src/adapter.ts

RodrigoAD · 2021-01-14T17:32:28Z

composite/bitcoin-json-rpc/src/adapter.ts

+  url: false,
+  method: false,
+  params: false,


url, method and params will be handled by JSONRPC adapter. You can just forward the input to the json adapter an it will handle it

I just left them here, so this composite adapter can inherit the actual functionality of the json-rpc one.

I reverted it now to a simpler version, that just accepts the endpoint param, for querying for blockchain stats, according to the scope of the ticket.

composite/bitcoin-json-rpc/src/adapter.ts

RodrigoAD · 2021-01-14T17:36:03Z

composite/bitcoin-json-rpc/src/adapter.ts

+  if (endpoint != undefined || blockchain != undefined) {
+    if (endpoint in convertEndpoint) endpoint = convertEndpoint[endpoint]
+    if (!endpoint) endpoint = 'difficulty'
+    request.data.method = 'getblockchaininfo'


We want method only to be 'getblockchaininfo' only if we don't have endpoint or blockchain params? Default values could come from makeConfig better.
Also, let's try to not modify directly the request object.

composite/bitcoin-json-rpc/src/adapter.ts

RodrigoAD · 2021-01-14T17:41:29Z

cryptoid/src/adapter.ts

+  const blockchain = validator.validated.data.blockchain.toLowerCase()
+
+  const key = util.getRandomRequiredEnv('API_KEY')
+  const q = endpointToQ[endpoint]


What is q? Could it have a more explanatory name?

q is a param of the cryptoid endpoint.

I would give the var an explicit name, even if it will be a param of the request, but we'll know what is it.

const someExplicitName = endpointToXXX[endpoint] const params = { key, q: someExplicitName }

RodrigoAD · 2021-01-14T17:44:51Z

cryptoid/src/index.ts

+import { makeExecute } from './adapter'
+import { makeConfig } from './config'
+
+const NAME = 'EXAMPLE'


I think you forgot to change the NAME

…s to json rpc adapter

…d of diffuculty endpoint and added height support

…ration

RodrigoAD

Thanks @ebarakos for the changes! Looking great, just some final notes

RodrigoAD · 2021-01-15T09:40:00Z

cryptoid/src/adapter.ts

+  const blockchain = validator.validated.data.blockchain.toLowerCase()
+
+  const key = util.getRandomRequiredEnv('API_KEY')
+  const q = endpointToQ[endpoint]


I would give the var an explicit name, even if it will be a param of the request, but we'll know what is it.

const someExplicitName = endpointToXXX[endpoint] const params = { key, q: someExplicitName }

composite/bitcoin-json-rpc/src/adapter.ts

composite/bitcoin-json-rpc/README.md

RodrigoAD · 2021-01-15T09:55:16Z

Take a look at the failing tests of bitcoin-rpc adapter, the build is trying to fetch a local node

RodrigoAD · 2021-01-15T10:19:42Z

cryptoid/src/adapter.ts

+  const blockchain = validator.validated.data.blockchain.toLowerCase()
+
+  const key = util.getRandomRequiredEnv('API_KEY')
+  const apiParam = endpointToApiParam[endpoint]


Sorry for being picky here, as I understand q param is the function you want to call from cryptoId, so something like cryptoIdFunction or apiFunction would be better.

q is the query parameter of the cryptoid endpoint. Not sure if it should be called function. In essence though, it has identical name as the JSON RPC methods.

I was thinking of something to let us know that the endpoint is mapped to the actual api's parameter.

Changed to apiFunctionName after discussion.

blockchair/src/endpoint/balance.ts

composite/bitcoin-json-rpc/README.md

composite/bitcoin-json-rpc/src/adapter.ts

composite/bitcoin-json-rpc/src/index.ts

composite/bitcoin-json-rpc/src/config.ts

krebernisak · 2021-01-15T10:40:13Z

cryptoapis/src/endpoint/stats.ts


-export const Name = 'difficulty'
+export const Name = 'stats'


Why do we name this as "stats" when we are actually calling Crypoapis /bc/.../info endpoint?

We should be calling it what it is, let's say blockchain-info or bc-info.

For Blockchair the endpoint is "stats", and we should call it "stats", but it's different for this adapter. When we get a query for BTC height, for Crypoapis adapter we need to call their bc-info endpoint and read the payload.blocks path from the response..

I was thinking of adhering to a certain name for this category of calls. Like we have balance and price, which also do not correspond to the actual endpoint naming.

Well just because we have some code implemented currently does not make it the correct way to do it :)

I suggest you rename this for now, and we'll make sure to improve/refactor other adapter implementations in the future.

"balance", "price", "difficulty" & "height" are all data points for which one can query our system. There is no "stats" data point currently. Here you are mapping the user request for "height" data point to a cryptoapis "bc-info" endpoint request.

Changed it to bc_info for now.

I think we have to find a uniform convention for these adapters that are returning blockchain info, while keeping difficulty (and now height) in backwards compatibility(probably by just delegating to stats/bc_info types when seeing these types of endpoint).

krebernisak · 2021-01-15T11:07:42Z

cryptoapis/src/endpoint/stats.ts

  blockchain: ['blockchain', 'coin'],
+  endpoint: false,


We should not reuse the name "endpoint" in our internal API. It just does not fit, just as it does not fit in the external API, but we are currently stuck with it there because of backward compatibility.

Internally, we should structure this differently.

So let's take this adapter and endpoint as an example, the "stats" endpoint.
The stats endpoint impl should have no knowledge of a concept like "height", this is a concept of our external API, it has nothing to do with "stats" endpoint. We should map a CL "height" request to a Cryptoapis "stats" request, but the impl for Cryptoapis "stats" request should have no knowledge of the outside CL "height" request.

The input to "stats" endpoint should look something like this:

const inputParams = { blockchain: true, network: false, path: false }

It makes a "stats" requests, and returns the data point at "path". If the "path" is empty, it returns the whole response body.

The component that is calling this should map from the CL API request to the "stats" API request. So your adapter in the top lvl execute function gets the input:

{ "blockchain": "btc", "endpoint": height }

We should map that to the underlying API endpoint fn call like this:

{ "blockchain": "bitcoin", "network": "mainnet", "path": "payload.blocks" }

@ebarakos @RodrigoAD @justinkaseman @emmick4 please acknowledge & discuss!

As this is a high priority for today, you can ignore this for this specific PR, but keep it in mind on the next one. I am also going to open an issue for it, to improve our older implementations.

cryptoid/src/adapter.ts

cryptoid/src/config.ts

composite/bitcoin-json-rpc/src/adapter.ts

cryptoapis/src/endpoint/bc_info.ts

cryptoid/src/adapter.ts

cryptoapis/test/stats.test.ts

blockchair/src/endpoint/stats.ts

cryptoapis/README.md

cryptoid/README.md

cryptoapis/README.md

krebernisak

Should be good to go!

A lot of valuable discussions in this PR 🙌 Thanks, @ebarakos 👍

ebarakos · 2021-01-15T15:58:03Z

Thanks for the thorough review @krebernisak and @RodrigoAD!

ebarakos force-pushed the feature/176305964-block-height-adapters branch from 0255ba3 to 44572aa Compare January 8, 2021 12:51

ebarakos marked this pull request as ready for review January 8, 2021 12:54

krebernisak suggested changes Jan 8, 2021

View reviewed changes

ebarakos changed the title ~~Added height(latest block number) as endpoint parameter on cryptoapis~~ Add block height in relevant adapters Jan 8, 2021

ebarakos requested a review from krebernisak January 11, 2021 13:18

ebarakos force-pushed the feature/176305964-block-height-adapters branch 8 times, most recently from 0042ba6 to 8ddf9f3 Compare January 14, 2021 15:14

RodrigoAD reviewed Jan 14, 2021

View reviewed changes

composite/bitcoin-json-rpc/src/adapter.ts Outdated Show resolved Hide resolved

RodrigoAD reviewed Jan 14, 2021

View reviewed changes

composite/bitcoin-json-rpc/src/adapter.ts Outdated Show resolved Hide resolved

RodrigoAD reviewed Jan 14, 2021

View reviewed changes

composite/bitcoin-json-rpc/src/adapter.ts Outdated Show resolved Hide resolved

RodrigoAD reviewed Jan 14, 2021

View reviewed changes

composite/bitcoin-json-rpc/src/adapter.ts Outdated Show resolved Hide resolved

RodrigoAD reviewed Jan 14, 2021

View reviewed changes

composite/bitcoin-json-rpc/src/adapter.ts Outdated Show resolved Hide resolved

RodrigoAD reviewed Jan 14, 2021

View reviewed changes

ebarakos added 4 commits January 15, 2021 10:05

Added height(latest block number) as endpoint parameter on cryptoapis

1f161ae

Added height endpoint for blockchair

82403e8

Added block height on endpoint on cryptoid

bcc9b3d

Add similar (to other blockchain stats querying adapters) input param…

d318c39

…s to json rpc adapter

ebarakos added 4 commits January 15, 2021 10:06

Changed blockchair according to the new changes. Created stats instea…

af23c5e

…d of diffuculty endpoint and added height support

Changed cryptoapis difficulty to stats and added height, after TS mig…

3d957cc

…ration

fixed README

eeb81c8

reverted to simpler version of bitcoin-json-rpc + review changes

edcff2a

ebarakos force-pushed the feature/176305964-block-height-adapters branch from 8ddf9f3 to edcff2a Compare January 15, 2021 08:57

ebarakos requested a review from RodrigoAD January 15, 2021 09:00

RodrigoAD reviewed Jan 15, 2021

View reviewed changes

reviews changed + reverted tests

6e70ec8

ebarakos requested a review from RodrigoAD January 15, 2021 10:19

RodrigoAD reviewed Jan 15, 2021

View reviewed changes

changed var name

a62df92

ebarakos force-pushed the feature/176305964-block-height-adapters branch from 6b06009 to a62df92 Compare January 15, 2021 11:01

krebernisak suggested changes Jan 15, 2021

View reviewed changes

Review changes

1ec7ed2

RodrigoAD reviewed Jan 15, 2021

View reviewed changes

cryptoid/src/adapter.ts Show resolved Hide resolved

cryptoid/src/config.ts Show resolved Hide resolved

reverted yarn.lock to 'develop' version

4ee780a

krebernisak suggested changes Jan 15, 2021

View reviewed changes

krebernisak reviewed Jan 15, 2021

View reviewed changes

cryptoid/README.md Outdated Show resolved Hide resolved

ebarakos added 2 commits January 15, 2021 17:28

review changes + updated README

fbe88b2

updated cryptoid README

eaf3831

ebarakos requested a review from krebernisak January 15, 2021 15:37

krebernisak reviewed Jan 15, 2021

View reviewed changes

cryptoapis/README.md Outdated Show resolved Hide resolved

Update cryptoapis/README.md

f2046a3

krebernisak approved these changes Jan 15, 2021

View reviewed changes

ebarakos merged commit e5aca08 into develop Jan 15, 2021

ebarakos deleted the feature/176305964-block-height-adapters branch January 15, 2021 16:12

krebernisak mentioned this pull request Jan 18, 2021

Improve how adapters structure and map internal API calls #241

Closed

ebarakos mentioned this pull request Jan 18, 2021

Cryptoapis tests and README #248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add block height in relevant adapters #215

Add block height in relevant adapters #215

ebarakos commented Jan 8, 2021

github-actions bot commented Jan 8, 2021

ebarakos commented Jan 8, 2021

krebernisak left a comment

krebernisak Jan 8, 2021

krebernisak Jan 8, 2021 •

edited

Loading

ebarakos commented Jan 8, 2021

ebarakos commented Jan 11, 2021

RodrigoAD Jan 14, 2021

ebarakos Jan 15, 2021 •

edited

Loading

RodrigoAD Jan 14, 2021

RodrigoAD Jan 14, 2021

ebarakos Jan 15, 2021

RodrigoAD Jan 15, 2021

RodrigoAD Jan 14, 2021

RodrigoAD left a comment

RodrigoAD Jan 15, 2021

RodrigoAD commented Jan 15, 2021

RodrigoAD Jan 15, 2021

ebarakos Jan 15, 2021

ebarakos Jan 15, 2021

krebernisak Jan 15, 2021

ebarakos Jan 15, 2021

krebernisak Jan 15, 2021 •

edited

Loading

ebarakos Jan 15, 2021

krebernisak Jan 15, 2021

krebernisak left a comment

ebarakos commented Jan 15, 2021

Add block height in relevant adapters #215

Add block height in relevant adapters #215

Conversation

ebarakos commented Jan 8, 2021

github-actions bot commented Jan 8, 2021

ebarakos commented Jan 8, 2021

krebernisak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krebernisak Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

ebarakos commented Jan 8, 2021

ebarakos commented Jan 11, 2021

Choose a reason for hiding this comment

ebarakos Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RodrigoAD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RodrigoAD commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krebernisak Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krebernisak left a comment

Choose a reason for hiding this comment

ebarakos commented Jan 15, 2021

krebernisak Jan 8, 2021 •

edited

Loading

ebarakos Jan 15, 2021 •

edited

Loading

krebernisak Jan 15, 2021 •

edited

Loading