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

feat: rpc upgrade #204

Merged
merged 7 commits into from
May 27, 2024
Merged

feat: rpc upgrade #204

merged 7 commits into from
May 27, 2024

Conversation

Keyrxng
Copy link
Member

@Keyrxng Keyrxng commented Mar 15, 2024

Relates to ubiquity/.github#100

  • removed chainlist submodule
  • import constants from package from constant.ts
  • create hook for use handler
  • temp fix to avoid error with ens lookup but expect that it'll need address once the UBQ RPCs are back up

using my package for sake of dev but once published I'll swap it out

@Keyrxng Keyrxng requested review from 0x4007 and rndquu as code owners March 15, 2024 15:59
Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems mostly fine but there were a lot more changes than I expected around the ENS lookup.

@Keyrxng
Copy link
Member Author

Keyrxng commented Mar 15, 2024

Seems mostly fine but there were a lot more changes than I expected around the ENS lookup.

The only reason being is because the UBQ RPCs are offline atm, I'll revert the changes back once those are back online if that's preferred

package.json Outdated Show resolved Hide resolved
@Keyrxng Keyrxng mentioned this pull request Mar 17, 2024
@molecula451
Copy link
Member

do you think this can help on #217 @Keyrxng ?

@Keyrxng
Copy link
Member Author

Keyrxng commented Apr 19, 2024

do you think this can help on #217 @Keyrxng ?

No it won't, you are correct we need to see mobile debug logs to see why the tx is failing.

The confusion it seems, is that all of the previous RPC issues and PRs have nothing to do with writing a tx on-chain. Allowance and BalanceOf are the only two functions which the 'optimized RPCs' affect.

The original need to optimize them came from the UI loading times which were significantly improved between myself and pav addressing CSS and unneeded fetch calls as well as preventing a UI glitch when any failed calls happened

@rndquu
Copy link
Member

rndquu commented May 20, 2024

@Keyrxng Could you:

  1. Fix merge conflicts
  2. Use https://www.npmjs.com/package/@ubiquity-dao/rpc-handler

@Keyrxng
Copy link
Member Author

Keyrxng commented May 20, 2024

Love to see it!

This PR improves the rpc-handler a lot, specifically it being used in other projects.

I've broken the PR into two, one for the tests and one for the polymorphic exports.

Because it introduces breaking changes via improved type exports it's probably best to update the rpc-handler first.

@rndquu
Copy link
Member

rndquu commented May 22, 2024

Love to see it!

This PR improves the rpc-handler a lot, specifically it being used in other projects.

I've broken the PR into two, one for the tests and one for the polymorphic exports.

Because it introduces breaking changes via improved type exports it's probably best to update the rpc-handler first.

https://www.npmjs.com/package/@ubiquity-dao/rpc-handler is updated

Now you're free to:

  1. Fix merge conflicts
  2. Use https://www.npmjs.com/package/@ubiquity-dao/rpc-handler

@rndquu rndquu marked this pull request as draft May 22, 2024 07:06
@Keyrxng Keyrxng marked this pull request as ready for review May 23, 2024 23:18
@Keyrxng Keyrxng requested a review from pavlovcik as a code owner May 23, 2024 23:18
@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented May 23, 2024

@Keyrxng
Copy link
Member Author

Keyrxng commented May 23, 2024

I've rerun the tests after each commit without error after updating the RPC endpoint. Could that workflow be run again with debug logging enabled pls

@gentlementlegen
Copy link
Member

I've rerun the tests after each commit without error after updating the RPC endpoint. Could that workflow be run again with debug logging enabled pls

Are you talking about the Cypress test workflow?

@Keyrxng
Copy link
Member Author

Keyrxng commented May 24, 2024

I've rerun the tests after each commit without error after updating the RPC endpoint. Could that workflow be run again with debug logging enabled pls

Are you talking about the Cypress test workflow?

I was @gentlementlegen, I haven't seen the nonce too low locally or otherwise before I don't think. The workflow is succeeding in another pull so it's likely a blip, could you fire it again with debug enabled and we'll see if it repeats it

@gentlementlegen
Copy link
Member

@Keyrxng https://github.com/ubiquity/pay.ubq.fi/actions/runs/9216125552/job/25421514595

Copy link
Contributor

github-actions bot commented May 26, 2024

@Keyrxng
Copy link
Member Author

Keyrxng commented May 26, 2024

race conditions avoided with a wait step added to the workflow which I'm sure should prevent the nonce too low error from happening again

Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine, RPC provider is fetched from https://www.npmjs.com/package/@ubiquity-dao/rpc-handler

@rndquu rndquu requested a review from gentlementlegen May 27, 2024 08:00
Copy link
Member

@gentlementlegen gentlementlegen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me too, please fix conflicts.

@Keyrxng
Copy link
Member Author

Keyrxng commented May 27, 2024

good to go

@rndquu rndquu merged commit f09e3da into ubiquity:development May 27, 2024
4 checks passed
@rndquu
Copy link
Member

rndquu commented May 27, 2024

@Keyrxng Tests in the development branch are failing after this PR is merged, could you fix it?

@rndquu
Copy link
Member

rndquu commented May 27, 2024

@Keyrxng Should we close ubiquity/.github#100 as completed?

@molecula451
Copy link
Member

great work @Keyrxng

@Keyrxng
Copy link
Member Author

Keyrxng commented May 27, 2024

-32003: nonce too low error again. I'm best guessing that the error is because one of the later tx's in the command is completing before an earlier one.

We could break apart the funding command into distinct steps and have a workflow step for each with a hardcoded wait in between.

Or a new script to handle the funding actions separately, waiting between each action and confirming it's affect?

Locally anvil.ts handles funding but it only handles the server instance in a workflow, so the workflow uses a yarn command to fund.


Since tests should be infallible I guess a couple error prone points should be taken care of:

  • anvil instance fails to launch due to RPC failure (happens locally from time-to-time, haven't seen it in a workflow run yet)
  • funding txs are fired in sequence with a retry mechanism in case of nonce too low or some other error
  • funding affect validation as it'll break tests if the funding address is double funded for example

@Keyrxng
Copy link
Member Author

Keyrxng commented May 27, 2024

@Keyrxng Should we close ubiquity/.github#100 as completed?

this is still open and this is the prod staking repo I'm sure so I'll tie this up and then it can be closed

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

Successfully merging this pull request may close these issues.

5 participants