-
Notifications
You must be signed in to change notification settings - Fork 300
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
Proof of Reserves batch requests & burst limiting #449
Conversation
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
|
||
const getBalanceURI = (address: string, confirmations: number) => | ||
`/q/addressbalance/${address}?confirmations=${confirmations}` | ||
const getBalanceURI = (addresses: string[]) => `balance?active=${addresses.join(',')}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const getBalanceURI = (addresses: string[]) => `balance?active=${addresses.join(',')}` | |
const getBalanceURL = (addresses: string[]) => `balance?active=${addresses.join(',')}` |
const getBalanceURI = (addresses: string[]) => `balance?active=${addresses.join(',')}` | |
const getBalanceURI = (addresses: string[]) => `balance?active=${addresses.join(',')}` |
* | ||
* @returns 2d array | ||
*/ | ||
export const chunk = (amount: number, data: any[]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use rxjs for this
return from(data).pipe(bufferCount(amount), toArray())
* @returns array of response data | ||
*/ | ||
export const throttle = async (amount: number, data: any[], exec: any) => { | ||
const chunks = chunk(amount, data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be expressed via rxjs
const output = from(data).pipe(
bufferCount(amount),
delay(1000),
mergeMap(
datas => forkJoin(datas.map(d =>exec(d)))
),
toArray()
).toPromise()
return output.flat()
} | ||
|
||
const response = await Requester.request(reqConfig) | ||
const response: any = await new Promise((resolve, reject) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use https://nodejs.org/api/util.html#util_util_promisify_original
here, or https://rxjs-dev.firebaseapp.com/api/index/function/bindNodeCallback
const requests = groups.flatMap((group) => { | ||
return getBalances(group as Account[], config) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about one-lining this?
const requests = groups.flatMap((group) => { | |
return getBalances(group as Account[], config) | |
}) | |
const requests = groups.flatMap((group) => | |
getBalances(group as Account[], config) | |
) |
const requests = groups.flatMap((group) => { | |
return getBalances(group as Account[], config) | |
}) | |
const requests = groups.flatMap((group) => { | |
return getBalances(group as Account[], config) | |
}) |
api.getAddrBal(account.address, params, (error: Error, body: AddressBalance) => | ||
error ? reject(error) : resolve(body), | ||
|
||
const _getAddrBal = (addrs: string[]) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Underscores mean unused in JS-land (by linters and tsc too), we shouldnt be using it as an internal function naming convention. Best to keep regular camel casing.
? await util.throttle(config.ratelimit, addresses, _getAddrBal) | ||
: await _getAddrBal(addresses) | ||
|
||
const addrLookup: { [key: string]: AddressBalance } = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const addrLookup: { [key: string]: AddressBalance } = {} | |
const addrLookup: Record<string,AddressBalance> = {} |
const addrLookup: { [key: string]: AddressBalance } = {} | |
const addrLookup: { [key: string]: AddressBalance } = {} |
const api = new bcypher(account.coin, chainId, config.apiKey) | ||
const getBalances: balance.GetBalances<Config> = async (accounts, config) => { | ||
const addresses = accounts.map((a) => a.address) | ||
const { coin, chain } = accounts[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it always the same coin/chain for all accounts? We should note that if so
export const makeConfig = (prefix = ''): Config => { | ||
const config = Requester.getDefaultConfig(prefix) | ||
const ratelimit = util.getEnv(ENV_RATE_LIMIT, prefix) | ||
if (ratelimit) config.ratelimit = Number(ratelimit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ratelimit) config.ratelimit = Number(ratelimit) | |
config.ratelimit &&= Number(ratelimit) |
if (ratelimit) config.ratelimit = Number(ratelimit) | |
if (ratelimit) config.ratelimit = Number(ratelimit) |
Closing. This PR is very outdated. The burst limiter middle-ware and batch warmer have made this change obsolete. |
Description
Replaying the previous #276 as a hotfix PR into master due to priority.
This will need to be engineered into an actual burst rate limit feature on EAEE.
Next steps:
API_RATE_LIMIT
will need to be added to Blockcypher's environment variables.API_KEY
andAPI_SECRET
need to be added for BTC.comBLOCKCYPHER_API_RATE_LIMIT
will need to be added to blockcypher PoR adapterBTC_COM_API_SECRET
andBTC_COM_API_KEY
will need to be added to BTC.com PoR adapterStory
https://app.clubhouse.io/chainlinklabs/story/7356/wbtc-por-adapter-fix