Skip to content
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

Proxy issues #56

Closed
whilefoo opened this issue Oct 10, 2024 · 6 comments
Closed

Proxy issues #56

whilefoo opened this issue Oct 10, 2024 · 6 comments

Comments

@whilefoo
Copy link

Because we are having problems with RPCs I looked at the code and found some quirks that I don't understand and some improvements that could be made.

@Keyrxng since you did this part, is there any reason why a JsonRpcProvider is wrapped with the Proxy two times, once here:
https://github.com/ubiquity/rpc-handler/blame/7a24b8a491695bb2121d01e44f4e7a4745ec61e7/types/rpc-handler.ts#L271
and here:
https://github.com/ubiquity/rpc-handler/blame/7a24b8a491695bb2121d01e44f4e7a4745ec61e7/types/rpc-handler.ts#L93
And is there a reason why we didn't just extend the BaseProvider from ethers?

Improvements:

  • When the RPC request fails it retries with the list of RPCs which includes this same RPC that failed, we should skip it instead.
  • If a RPC fails a couple of times even across different requests, we can mark it as offline and not use it anymore
@0x4007
Copy link
Member

0x4007 commented Oct 10, 2024

Hey I just totally redid it

Notes:

It's really solid after first time initalization for a few seconds. then the fastest network is cached.

The test is way more robust, it actually downloads the smart contract code of permit2. The RPC endpoints cant lie about that response, like block height. They have to actually read the chain data correctly

https://github.com/0x4007/rpc2

Will install and test in pay.ubq.fi after I get some rest. If you're motivated you can work on integrating it for the next 12 hours or so.

@whilefoo
Copy link
Author

How come you didn't use JsonRpcProvider from ethers? Without that we have to manually encode and decode data

@0x4007
Copy link
Member

0x4007 commented Oct 11, 2024

I can probably make an adapter quite easily when I have more time

@Keyrxng
Copy link
Member

Keyrxng commented Oct 15, 2024

Sorry guys I'll be back home tonight.

Probs missed it, but as far as I know pay.ubq.fi is running on V1.1 while this package currently sits at V1.3 so the actual proxy is not being used in pay.ubq.fi, unless these issues are occurring elsewhere or it was bumped in pay.ubq.fi and then reverted?

@whilefoo I'm sure it was because testRpcPerformance() can be called on it's own and it's possible that no class provider is set but looking at things it could be removed and that function made private as it's internal anyway after seeing the handler in use

@Keyrxng
Copy link
Member

Keyrxng commented Oct 25, 2024

Same applies here as in this comment

@whilefoo testRpcPerformance() is used here in pay.ubq.fi as part of spawning Anvil during CI so the fn should remain public but we can remove the double Proxy wrap if you think we should.

Do you feel it'd be better/cleaner to extend the BaseProvider and create our own? I hadn't looked into it but assumed we'd need to rewrite encode/decode or at least some logic and didn't think that was ideal but if we open a task for it I'd be happy to look into it unless someone else wants to.

@whilefoo
Copy link
Author

Do you feel it'd be better/cleaner to extend the BaseProvider and create our own? I hadn't looked into it but assumed we'd need to rewrite encode/decode or at least some logic and didn't think that was ideal but if we open a task for it I'd be happy to look into it unless someone else wants to.

It sounds like it's cleaner to extend BaseProvider but I'm not sure how much work is that and how much abstraction we lose, maybe extending JsonRpcProvider could be another option

@Keyrxng Keyrxng closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants