Proof of Reserve batch requests #276
Conversation
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
justinkaseman
force-pushed
the
feature/PoR-batching
branch
from
7956e4f
to
a33b6cf
Compare
| import { isChainType, isCoinType } from '.' | ||
| import * as blocktrail from 'blocktrail-sdk' |
There was a problem hiding this comment.
Blocktrail is now btc.com?
If this is true could we document it somehow, as it's confusing reading it as is.
There was a problem hiding this comment.
What would be the benefit of accessing the API through this SDK?
There was a problem hiding this comment.
Blocktrail is now btc.com?
If this is true could we document it somehow, as it's confusing reading it as is.
I'm not sure how this is all settling on their side.
Blocktrail's website now links to BTC.com and BTC.com's new docs say to use the SDK.
Their support email shows their name as bitmain.
What would be the benefit of accessing the API through this SDK?
Originally I did it because of https://dev.btc.com/docs/js#api-authentication (they haven't documented how to authenticate so I just went through the SDK).
I could go back now and try sending the API_KEY and API_SECRET as a param.
blockcypher/src/endpoint/balance.ts
Outdated
| * | ||
| * @returns array of response data | ||
| */ | ||
| const throttle = async (amount: number, data: any[], exec: any) => { |
There was a problem hiding this comment.
Rate limiting, not throttle is the concept we are looking for.
Research rate limit algorithms:
- https://blog.logrocket.com/rate-limiting-node-js/
- https://kendru.github.io/javascript/2018/12/28/rate-limiting-in-javascript-with-a-token-bucket/
- https://aliz.ai/rate-limiting-in-rxjs/
Pick one and implement it to be used like this:
const count = 5
const slidingWindowTime = 1000
const _echo = (msg) => console.log(msg)
const _limitedEcho = utils.rateLimit(count, slidingWindowTime, _testFn)
while (true) _limitedEcho("Hello!") // This should write 5x "Hello!", then sleep until the end of the sliding window, and repeat indefinitelyPlease also document the algorithm you picked and implemented.
There was a problem hiding this comment.
Thanks for pointing me in the right direction with the name 😅
I renamed this to rate_limit.
How do you feel about moving this re-usable implementation to a separate issue? PoR is a high priority item for the business side and I worry making this change might take a little bit to implement.
| export type GetBalance<C extends Config = Config> = ( | ||
| account: Account, | ||
| config: BalanceConfig<C>, | ||
| ) => Promise<BalancesResponse> | ||
| export type GetBalances<C extends Config = Config> = ( | ||
| accounts: Account[], | ||
| config: BalanceConfig<C>, | ||
| ) => Promise<BalancesResponse> | ||
|
|
||
| export type BalanceConfig<C extends Config = Config> = C & { | ||
| confirmations?: number | ||
| isSupported: IsSupported | ||
| getBalance?: GetBalance | ||
| getBalances?: GetBalances | ||
| getBalance?: GetBalance<C> | ||
| getBalances?: GetBalances<C> | ||
| } |
There was a problem hiding this comment.
I assume you noticed there is something wrong here 😅
This exploded in complexity, unnecessarily IMHO.
A hint you can use is that the API should probably be amended to look more like this:
export type GetBalances = (accounts: Account[], confirmations?: number) => Promise<BalancesResponse>We don't configure the function calls, we configure the adapter. Also not sure we even need the single returning GetBalance, as we can support it all by using GetBalances.
There was a problem hiding this comment.
This happened because the Config needed to be extended to include new per provider env variables such as API_SECRET. This Config gets passed through the factory, so I made everything a generic to take in possibly extended Conifgs, but also fall back to the default Config type if not given.
Not sure that I understand how we could make the GetBalances balance request without the Config, but I'll put some thought into how I could refactor & clean this up.
justinkaseman
force-pushed
the
feature/PoR-batching
branch
from
f55fdba
to
011b81d
Compare
justinkaseman
force-pushed
the
feature/PoR-batching
branch
from
011b81d
to
4f70c11
Compare
Description
Further investigation into using batched requests concludes that:
amberdata - no batching
blockchain_com - supports batching
blockcypher - supports batching
blockchair - supports batching
btc_com - no batching
cryptoapis - no batching
sochain - no batching
This PR makes the following changes:
BTC.comrecently added API_KEY and API_SECRET. The adapter has been changed to use their SDK.balanceea-factory and methods are made into generics, taking an extended Config if needed, but defaulting to the normal Config type.blockchain.comandblockcypher(blockchairadapter already uses it)blockcypher's strict request per second restrictions adds the ability to throttle the number of requests per second. Setting this to3works on the free tier key, but takes ~10 seconds to finish forwbtc.Issues
#183
#280