ubiquity / pay.ubq.fi

Generate and claim spender permits (EIP-2612)
https://pay.ubq.fi
8 stars 28 forks source link

RPC does not support `eth_call` method #235

Closed Keyrxng closed 3 weeks ago

Keyrxng commented 4 weeks ago

When attempting to claim, an error is thrown in isNonceClaimed on one provider I have found so far stating the method eth_call does not exist/is not available.

This script will succeed just as often as it will fail. I thought it was the endpoint being out of sync but if the block numbers match that's less likely to be the case. It's a valid endpoint as it always responds with a block number and that's how we determine if an RPC is good or bad, so excluding doesn't make sense.


Calling isNonceClaimed() inside useRPCHandler() could allow us to either select a new provider or retry until it succeeds (it can fail consecutively, so replacing is better) and remove the call to isNonceClaimed() inside checkPermitClaimable() removing the block.

Either that or create a some kind of fallback to retry then reselect a new endpoint should an error occur and retry the same call looping the other providers until it does succeed? This function could be re-used across all of the onchain calls.


Current effects are a console-only error log and no user feedback followed by the claim button being removed. Current solve is to refresh hoping for a new provider or that same provider will handle the request successfully.

reproduce on prod with this permit, a fake one from V2 testing. It'll say "no funds let funder know" if no problems otherwise it'll log the error.

success and failure log ``` 14:29 ❯ npx tsx t.ts { ethCallData: { id: 1, jsonrpc: '2.0', result: '0x0000000000000000000000000000000000000000000000000000000000000000' } } 14:29 ❯ npx tsx t.ts { ethCallData: { id: 1, jsonrpc: '2.0', error: { message: 'the method eth_call does not exist/is not available', code: 32601 } } } ```
the script ```ts const badRpc = "https://gnosis.drpc.org"; const goodRpc = "https://gnosis-pokt.nodies.app"; const nonceBitmapData = { to: "0x000000000022D473030F116dDEE9F6B43aC78BA3", data: "0x4fe02b44000000000000000000000000d9530f3fbbea11bed01dc09e79318f2f20223716001fd097bcb5a1759ce02c0a671386a0bbbfa8216559e5855698a9d4de4cddea", accessList: null, }; type Resp = { jsonrpc: string; result: string; id: number; }; const reqMaker = (rpc: string, method: string, params: any[]) => fetch(rpc, { method: "POST", headers: { "Content-Type": "application/json", }, body: JSON.stringify({ jsonrpc: "2.0", id: 1, method, params, }), }); const getBlockNumber = (rpc: string) => reqMaker(rpc, "eth_blockNumber", []); (async () => { const goodBlockNo = new Promise((resolve, reject) => { getBlockNumber(goodRpc) .then((res) => res.json()) .then((data) => resolve(data)) .catch((err) => reject(err)); }); const badBlockNo = new Promise((resolve, reject) => { getBlockNumber(badRpc) .then((res) => res.json()) .then((data) => resolve(data)) .catch((err) => reject(err)); }); const [goodBlockNoData, badBlockNoData] = await Promise.all([goodBlockNo, badBlockNo]); if (goodBlockNoData.result === badBlockNoData.result) { const ethCallResponse = await reqMaker(badRpc, "eth_call", [nonceBitmapData, "latest"]); const ethCallData = await ethCallResponse.json(); console.log({ ethCallData }); } })().catch(console.error); ```
time <3hr
0x4007 commented 4 weeks ago

Shouldn't this be handled by the NPM module instead of the application?

Keyrxng commented 4 weeks ago

Shouldn't this be handled by the NPM module instead of the application?

No and for a couple of reasons and those are that the RPC is responding as it should to a eth_blockNumber call quickly with valid output, it is only running into an error when attempting to make an eth_call request and only occasionally does it happen so all in all it is a working endpoint. For the module to handle this, it would need to fire an eth_call request and even then it may succeed and slip through only to fail on the UI on the user invoked eth_call attempt.

StackEx etc point to the provider being out of sync, if that's the case there isn't anything we can do other than retry a few times and replace the endpoint.


Also the provider API is standardized to a point and some RPCs expose methods that others don't which was my very first thought on why it was failing. But overall I think it makes sense for the app to handle this

In testing I'm seeing it can happen to more than one RPC, and then will succeed with that same RPC on the next pass. I don't think the module can exclude based on this because of the hit rate as it would still be possible to fail

I'm trying to understand what the underlying issue is that's causing it if it's not a provider sync issue

❯ npx tsx t.ts
Trying to call https://gnosis.drpc.org
Called failed with https://gnosis.drpc.org and error the method eth_call does not exist/is not available
Trying to call https://gnosis-pokt.nodies.app
Called failed with https://gnosis-pokt.nodies.app and error the method eth_call does not exist/is not available
Trying to call https://gnosis.drpc.org
Called failed with https://gnosis.drpc.org and error the method eth_call does not exist/is not available
Trying to call https://1rpc.io/gnosis
Call succeeded with https://1rpc.io/gnosis
Result 0x0000000000000000000000000000000000000000000000000000000000000000

❯ npx tsx t.ts
Trying to call https://gnosis.drpc.org
Call succeeded with https://gnosis.drpc.org
Result 0x0000000000000000000000000000000000000000000000000000000000000000
rndquu commented 4 weeks ago

One of the reasons for does not exist/is not available kind of RPC errors is that node is not synced which is pretty usual for new node infrastructure providers. I remember running a private PoA network via geth and nodes were out of sync way too often. I guess for public nodes the situation is even worse.

Anyway I also think that does not exist/is not available error should be handled in https://github.com/ubiquity/rpc-handler instead of reimplementing the rpc-handler module's logic in all of the apps that use @ubiquity-dao/rpc-handler.

So https://github.com/ubiquity/rpc-handler should either change RPC provider under the hood in case there is smth wrong either emit a "found better provider RPC" event so that client apps could change the provider.

Keyrxng commented 4 weeks ago

Because the module returns a provider to the app and then the app pushes calls through the provider and not the module, the module does not know that the eth_call attempt failed and so would require a call during a try{}catch{} or similar during any provider call attempt back into the module to select a new provider which the app would assign globally and retry the call.

I guess the Proxy approach could be adopted inside the module so that it returns a proxy to the JsonRpcProvider rather than the provider itself but then it means package users don't have a much say in error handling.

Since in tests it was shown to fail with RPC A during call 1 and then succeed with that same RPC during call 2, say the module says RPC A is valid and the app calls it and it fails on call 2 how would that be handled from the app other than what I mentioned above? As each call to the provider is a separate function throughout the codebase every fn call would need wrapped in this reselectNewProvider() wrapper/fallback wouldn't it?


If the module should attempt to cover this and exclude RPCs based on this error, should we use this nonceBitmap call to be the deciding factor in the module as to whether or not the RPC is valid then?

My comment about RPC method exposure comes to mind,eth_call is a standard method and should be covered but should we also aim to extend the module to be able to filter providers based on a particular method being valid beyond eth_call?

reimplementing the rpc-handler module's logic

I see what you both mean here although I thought the purpose of the module was to filter RPCs which were outright non-starters and so long as they responded with a block number they were considered valid.

Keyrxng commented 4 weeks ago

Trying to call https://gnosis-pokt.nodies.app Called failed with https://gnosis-pokt.nodies.app and error the method eth_call does not exist/is not available

My concerns come from the fact that this is one of the better RPCs and even it failed at one point. So to avoid the 'first call succeeds, second call fails' problem say we try the call 5 times for each provider, there's a high chance that it'll fail once for most, if not all of them and if it doesn't it still likely could.

So it comes down to I think, should the module just be returning a supply of providers and package users handle their own reselection and retry logic or should txs be pushed through it like with this Proxy approach, so that it handles the reselection directly?

So https://github.com/ubiquity/rpc-handler should either change RPC provider under the hood in case there is smth wrong either emit a "found better provider RPC" event so that client apps could change the provide

rpc-handler returns a list of RPCs which respond with a valid block number, if we introduce an eth_call check and let's say the returned list is non-null, rpc-handler is hands-off from here as mentioned above it doesn't know if the user invoked call failed until the app calls into the module again via getLatencies() or getFastest...(). So the module as-is shouldn't and can't know when to say "change rpc this one is bad" unless we change to pushing txs through it. It just says "heres a list of RPCs that 100% work as far as receiving a healthy response is concerned" and like you say we are still at the whim of public nodes, so they are not 100% infallible

0x4007 commented 3 weeks ago

The RPC handler module should abstract all of the headaches of dealing with the RPCs away from the application level developer. It should automatically handle fallbacks as needed, and its API should abstract all of this away from the application developer.

Keyrxng commented 3 weeks ago

The RPC handler module should abstract all of the headaches of dealing with the RPCs away from the application level developer. It should automatically handle fallbacks as needed, and its API should abstract all of this away from the application developer.

I think then that the handler should just return a Proxy to the provider and all txs regardless of where in the app they are called will be pushed through the module, the module will then retry until success or configurable conditions are met.

This way there is no change at all to how the handler is originally called before this PR, the Proxy creation etc would be handled inside the module and also means there's no requirement to wrap every call your app might have in a fallback/retry.

Perhaps it should expose both a stress-free everything-handled-for-you Proxy and non-proxy, either they want to handle it themselves and can or they don't.

rndquu commented 3 weeks ago

I guess the Proxy approach could be adopted inside the module

+1

but then it means package users don't have a much say in error handling

I’m sure there is a way to provide package users an ability to catch module errors

If the module should attempt to cover this and exclude RPCs based on this error, should we use this nonceBitmap call to be the deciding factor in the module as to whether or not the RPC is valid then?

I don’t understand how nonceBitmap is connected with selecting RPC node

Perhaps it should expose both a stress-free everything-handled-for-you Proxy and non-proxy, either they want to handle it themselves and can or they don't.

Sounds good, especially for backwards compatibility since https://github.com/ubiquity/rpc-handler is already used at least by pay.ubq.fi

Keyrxng commented 3 weeks ago

I don’t understand how nonceBitmap is connected with selecting RPC node

It's not tied to a nonceBitmap call specifically it will likely do the same thing with any eth_call payload but because it was the failing call on the UI I thought to just replicate these failing conditions.

Although if this proxy approach is adopted in the module there isn't really a need to do this eth_call check.

I’m sure there is a way to provide package users an ability to catch module errors

The only errors that the module should emit I think are user-error as the module should handle anything out of the user's control by the sounds of it.

I have opened a new issue for it which I think covers everything needed

rndquu commented 3 weeks ago

@Keyrxng So this issue should be closed in favor of https://github.com/ubiquity/rpc-handler/issues/23, right?

Keyrxng commented 3 weeks ago

Yeah I'm working rpc-handler now and there shouldn't be any change needed in this codebase, other than a package bump ofc

ubiquibot[bot] commented 3 weeks ago
# Issue was not closed as completed. Skipping.
ubiquibot-v2-testing[bot] commented 3 weeks ago

[ 4.65 WXDAI ]

@0x4007
Contributions Overview
View Contribution Count Reward
Issue Comment 2 4.65
Conversation Incentives
Comment Formatting Relevance Reward
Shouldn't this be handled by the NPM module instead of the appli…
1.2
p:
  count: 12
  score: 1
0.8 0.96
The RPC handler module should abstract all of the headaches of d…
4.1
p:
  count: 41
  score: 1
0.9 3.69

[ 250.46 WXDAI ]

@Keyrxng
Contributions Overview
View Contribution Count Reward
Issue Specification 1 56.426
Issue Comment 6 194.034
Conversation Incentives
Comment Formatting Relevance Reward
When attempting to claim, an error is thrown in `isNonceClaimed`…
63.4
p:
  count: 434
  score: 1
code:
  count: 198
  score: 1
a:
  count: 2
  score: 1
0.89 56.426
No and for a couple of reasons and those are that the RPC is res…
79.2
p:
  count: 310
  score: 1
code:
  count: 86
  score: 1
0.86 68.112
Because the module returns a provider to the app and then the ap…
62.4
p:
  count: 306
  score: 1
code:
  count: 6
  score: 1
0.83 51.792
My concerns come from the fact that this is one of the better RP…
47.2
p:
  count: 231
  score: 1
code:
  count: 5
  score: 1
0.8 37.76
I think then that the handler should just return a `Proxy` to th…
22.6
p:
  count: 112
  score: 1
code:
  count: 1
  score: 1
0.81 18.306
It's not tied to a `nonceBitmap` call specifically it will likel…
20.4
p:
  count: 99
  score: 1
code:
  count: 3
  score: 1
0.76 15.504
Yeah I'm working `rpc-handler` now and there shouldn't be any ch…
3.2
p:
  count: 15
  score: 1
code:
  count: 1
  score: 1
0.8 2.56

[ 16.128 WXDAI ]

@rndquu
Contributions Overview
View Contribution Count Reward
Issue Comment 3 16.128
Conversation Incentives
Comment Formatting Relevance Reward
One of the reasons for `does not exist/is not available` kind of…
13.4
p:
  count: 120
  score: 1
code:
  count: 13
  score: 1
a:
  count: 1
  score: 1
0.9 12.06
+1 I’m sure there is a way to provide package users an ability t…
4.5
p:
  count: 43
  score: 1
code:
  count: 2
  score: 1
0.72 3.24
@Keyrxng So this issue should be closed in favor of https://gith…
1.2
p:
  count: 12
  score: 1
0.69 0.828