429 errors bubbling up too high about js-tableland HOT 13 CLOSED

For the Notion issue, it looks like it might be the headings that are unsupported. On #84 I did an edit to get rid of a heading and that changed the status from not working to working. This is too bad because you have headings in your issue template!

This great, I'm gonna open another issue (both to separate and as a test) for the notion integration problems. 🙏

from js-tableland.

I'm getting multiple failure notices from GitHub Actions on this issue, of the form:
Add GitHub Issues to Notion ERROR: Unsupported markdown element: ...
Bugs in the bug reporter make it hard to raise an issue!

from js-tableland.

Hey @wbt Thanks for opening this! Sounds like you have two issues here. One is with create and one is with using unsupported markdown, is that right?

For the first issue, it looks like an issue with Metamask. Do have any more details on what you are trying to do? The message makes it seem like you are making more than 40 requests per second which is quite a lot. I don't think it would make sense for this module to automatically retry creating a table if Metamask is returning a rate limit exceeded error, but I'm curious what your use-case is.

For the second issue with markdown, any suggestions on what text should be allowed that currently is not?

from js-tableland.

I think there are at least three issues here:

1. MetaMask is making too many requests to the JSON RPC node to check on transaction status, and hitting a public API rate-limit. That's not something I'm choosing, that's either MetaMask or TableLand code checking on things too frequently. There is nowhere in my application code (that I know of) where I can control this, as it still happens even when running through just one transaction at a time and waiting for that to complete before seeking any other information or transactions.
   Though it is an initial trigger here, I'm not actually that worried about that error, because it shouldn't be fatal. If one check hits a rate limit, code should be able to just wait for the next one - and sometimes it does, but sometimes not.

2. Tableland fails to handle the error from the above issue gracefully. This is an issue which should be fixed in Tableland code, and that's what this GitHub issue should focus on.

3. The original issue report above has the markdown that isn't being parsed correctly by Notional. I don't think it's that fancy, but it does use a few formatting features. The error doesn't have enough to help me figure out easily what Markdown code is actually causing the error. This probably has to be fixed in Notional code for a long-term fix, but disabling the "feature" of having Notional throw more errors at anybody trying to report an issue here should probably be disabled until they report they've fixed that. This is probably something you should report to Notional since you're their direct customer.

from js-tableland.

Concerning issue 1, this seems like it might be a misconfiguration of Metamask. I don't come up with anything useful searching for that error, and it seems odd to make more than 40 requests per second. Maybe try with the brave browser wallet? Regardless we can leave that for now.

For issue 2. I don't think it makes sense for the Tableland JS SDK to hide errors and retry sending transactions, but I could be wrong. If you can provide any kind of working example of the issue and what you would expect that would help us understand what you're expecting?
To be specific on my thinking, Tableland's create method is not designed to work only with Metamask. The goal is as many wallets as reasonable. If the error is originating in Metamask, I'm assuming this line is the relevant part of the source. Doing some kind of try, catch, and check the error and retry here will get very messy and quickly become unmaintainable. Also, if a different app does not want that functionality it would be an issue for them if the SDK forces it to happen.
I could be misunderstanding what you're suggesting, if so please feel free to elaborate more or open a PR as a way to discuss.

For issue 3, I don't feel that I would be able to open an issue with Notion without some kind of example. If you have time to provide a reproduce-able error, that would be helpful.

from js-tableland.

For issue 1, I don't know where to go in searching for a fix to that, or if it might be connected to having too many tabs open in the browser, but I think that much should be pretty much out of scope for this Issue.

For issue 2, I think it does make sense for TableLand to hide errors that don't indicate the transaction has actually encountered an error, but rather only mean one particular status check failed due to a temporary condition (API rate limit).
Requiring the try/catch/retry messiness in user code gets even messier, and (for some context) the fact that this requirement isn't documented is one of the reasons why I spent most of HackFS fighting with this (and other Tableland issues like it) instead of being able to actually get the intended functionality implemented and be able to integrate the other event technologies as required for prize eligibility. I saw others in chat getting hampered by this too.

For issue 3, there is a reproducible example in the original post just above. For convenience, here is the markdown copy-pasted:

## Expected Behavior
A tableland call (e.g. `create()`) which triggers a transaction that succeeds doesn't throw an error

## Actual Behavior
Sometimes, MetaMask encounters an error `inpage.js:1 MetaMask - RPC Error: Non-200 status code: '429' details: 'Rate limit exceeded: 40 per 1 second. Check respon…ticvigil.com/ to avoid hitting public ratelimits.', code: -32005,` when checking the transaction status, and that particular status check fails.  This error should be caught in the code which waits for a response, and continue waiting, as a subsequent status check can still succeed and reveal that the transaction was successfully mined into a block.  The error should not be bubbled up to the calling context of `create()`.

## Steps to Reproduce the Problem
1. In code running in a browser with MetaMask, connect to tableland and make a `create()` call from the SDK.   It is an intermittent issue affecting <50% of calls, but still problematic.

The workaround is to catch the error in the calling context and re-trigger creation of the table, but the gas/resources wasted on successfully creating the first are lost due to poor error handling in the SDK.

## Specifications

- Version: @tableland/sdk@2.0.2
- Platform: Chrome on Windows

from js-tableland.

For the Notion issue, it looks like it might be the headings that are unsupported. On #84 I did an edit to get rid of a heading and that changed the status from not working to working. This is too bad because you have headings in your issue template!

from js-tableland.

For issue 2, I think it does make sense for TableLand to hide errors that don't indicate the transaction has actually encountered an error, but rather only mean one particular status check failed due to a temporary condition (API rate limit).
Requiring the try/catch/retry messiness in user code gets even messier, and (for some context) the fact that this requirement isn't documented is one of the reasons why I spent most of HackFS fighting with this (and other Tableland issues like it) instead of being able to actually get the intended functionality implemented and be able to integrate the other event technologies as required for prize eligibility. I saw others in chat getting hampered by this too.

Sorry to hear you had issues during HackFS. Tableland is very new and you being willing to try it and help work out the kinks is much appreciated.
Regarding this library hiding errors from the caller, in my experience letting the caller handle errors will be much better in the long run. If an application is making more than 40 requests per second it seems like the application has a bug or is incorrectly using the SDK. If the SDK hides rate limit errors from the application it is worse for all parties involved, i.e. metamask gets hammered with requests and the SDK user never knows that their app is not working right.

With that said, let's leave this open to allow others to comment or potentially submit a PR.

from js-tableland.

If an application is making more than 40 requests per second it seems like the application has a bug or is incorrectly using the SDK.

I'm not directly making any requests to MetaMask or the SDK to check on transaction status. I'm using await on the call to TableLand, letting Tableland or MetaMask decide how often to poll (if it even uses polling) or whether to use some event-driven subscription model that doesn't require polling. I don't think there is anything I can do in my code to reduce the rate of that checking, so I don't think the quoted sentence is correct.

from js-tableland.

As mentioned above this error should be handled by the application.
Closing as there's been no new activity here.

from js-tableland.

Tableland is the application here - that's where it should be handled. Tableland is what reports the transaction as having failed, requiring another call to repeat the transaction in Tableland, when the original might still succeed. Duplicating the action is sometimes very costly and undesirable - the user wants to execute the action just once.

from js-tableland.

To be clear on my meaning of application, Tableland is a library or SDK, an application is the code base that is importing this module.
For this module to hide critical errors coming back from metamask, especially rate limit errors, would be a mistake.
@wbt If you want to submit a pull request with a fix, or a reproducible error, we can reopen.

from js-tableland.

The text here and in the linked MetaMask issue contain enough information to reproduce, though it's only occasional reproduction unless you also run the RPC endpoint and inject your own 429s for testing.

Getting this failure bubbled up from Tableland, an application is receiving the message that the Tableland transaction failed, prompting the application to re-initiate a new transaction that takes the same action again. Depending on the action, that can be very costly and undesirable. Tableland should only be reporting a failure if the transaction actually fails. I don't think that's too crazy a developer expectation!

from js-tableland.