ubiquity / pay.ubq.fi

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

feat: rpc upgrade #204

Closed Keyrxng closed 1 month ago

Keyrxng commented 3 months ago

Relates to https://github.com/ubiquity/.github/issues/100

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

Keyrxng commented 3 months ago

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

molecula451 commented 2 months ago

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

Keyrxng commented 2 months ago

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 commented 1 month ago

@Keyrxng Could you:

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

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 commented 1 month ago

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
ubiquibot-continuous-deploys[bot] commented 1 month ago
9e35444
2f30744
a31dbbe
f09e3da
Keyrxng commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

github-actions[bot] commented 1 month ago
Preview Deployment
[2f3074409b302e86d752105fc715368dceb50be3]()
[a31dbbe81623963ec9c9b1ec5fb5323b0efa2a0a]()
[f09e3da2f5f2b413a2a6746c2ecc1502ce0065a4]()
Keyrxng commented 1 month ago

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

Keyrxng commented 1 month ago

good to go

rndquu commented 1 month ago

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

rndquu commented 1 month ago

@Keyrxng Should we close https://github.com/ubiquity/.github/issues/100 as completed?

molecula451 commented 1 month ago

great work @Keyrxng

Keyrxng commented 1 month ago

-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:

Keyrxng commented 1 month ago

@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