ArweaveTeam / arweave-js

Browser and Nodejs client for general interaction with the arweave protocol and gateways
MIT License
594 stars 129 forks source link

getPrice can return non-integer "Too Many Requests", should throw #183

Closed vird closed 1 year ago

vird commented 2 years ago

https://github.com/ArweaveTeam/arweave-js/blob/a07243c0e418a4e8d7ecc4afa0a1aa55e8fb2acc/src/common/transactions.ts#L55

rosmcmahon commented 2 years ago

you could determine if it's parseable to an int? if you need an error, you could then create one, would this be reasonable? :

const price = parseInt(await getPrice(123))
if(isNaN(price)) throw new Error("error fetching price")
vird commented 2 years ago
  1. getPrice call is inside createTransaction https://github.com/ArweaveTeam/arweave-js/blob/a07243c0e418a4e8d7ecc4afa0a1aa55e8fb2acc/src/common/common.ts#L124 So I need check each createTransaction for if (isNaN(tx.reward)) in all places. I preffer createTransaction which should either generate valid transaction or throw exception

  2. Also this bug is actually not a bug of library but bug on arweave.net gateway, because it should have error code 429, but normal code returned

  3. I've made this fix for my purposes https://github.com/virdpool/arweave-js/commit/91a7e04628d2aad94baa1cf81dc65b6be665ae04

rosmcmahon commented 2 years ago

This ties in with 2 or 3 other open issues regarding lack of error propagation. It's not a quick fix, so someone needs to spend a few hours on this.

rosmcmahon commented 1 year ago

right, so in switching over to fetch it has become apparent why this was set up in this way. some work had already been done in preparing for a switch over, fetch doesn't throw errors for status codes.

i was trying to recreate your error, and i realised that there is an incredibly high rate limit for this endpoint.

what are you doing that requires creating more than 30,000 transactions every 5 minutes @vird ? it's not possible to post them, that's for sure!

vird commented 1 year ago

Global rate limit among all routes - 100 per second (30,000 per 5min)

I don't create too much transactions, but other endpoints are heavily used. So transaction creation could fail. Usually there is no problem with 5 min limit, but there is problem with per second limit. Imagine pool found a block and block is confirmed. In worst case I need to send transaction to all miners. If there is more than 100 miners (different addresses) I need to send 100 transactions. And I need store that transactions to calculate fee for each transaction to make sure that there is no problem with balance overuse (otherwise last tx will fail).

But actually I didn't make deep investigation how many request I do to arweave.net. I have no monitoring inside or outside arweave-js. And now I can't recheck that because pool hashrate is low

rosmcmahon commented 1 year ago

hmm. ok. i'll leave this issue open for now, there's another unhandled error issue opened. likely they will both get looked at at the same time, when someone has free time.

rosmcmahon commented 1 year ago

putting in a fix for this now. stole your regex, thanks!