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

refactor: remove getWeb3 #1837

Merged
merged 4 commits into from
Apr 5, 2023
Merged

refactor: remove getWeb3 #1837

merged 4 commits into from
Apr 5, 2023

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Apr 3, 2023

What it solves

Resolves #1799

How this PR fixes it

  • getWeb3 has been removed and the provider is now drilled accordingly.
  • Contract instances have been checked and altered to their read-only counterpart accordingly.

How to test it

  • (Relayed) Safe creation (incl. gas estimation) should work, setting the Fallback Handler correctly.
  • Creating/executing/relaying (batched) transaction(s) should work:
    • Simulation should work.
    • Validity of transaction(s) should pass (no warning shown that the transaction may fail).
  • Safe updates should work.
  • Spending limits should work:
    • Loading of spending limit balancess should work.
    • Removal of spending limits should work.
  • Signing on-chain messages via Safe Apps should work.

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

@iamacook iamacook self-assigned this Apr 3, 2023
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

Branch preview

✅ Deploy successful!

https://remove_getweb3--webcore.review-web-core.5afe.dev

@github-actions
Copy link

github-actions bot commented Apr 3, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@iamacook iamacook changed the title [WIP] refactor: remove getWeb3 refactor: remove getWeb3 Apr 4, 2023
@iamacook iamacook marked this pull request as ready for review April 4, 2023 08:31
Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@francovenica
Copy link
Contributor

I have issues with goerli. I don't know if this ticket deals with networks or not but I want to point it out still

With the stg CGW I cannot create safes in goerli
With the stg or prod CGW I cannot execute tx with the tx builder app in goerli

once the sponsored tx run out and I have to use my own funds, I can create the safe and execute the tx with the tx builder just fine.

I didn't have this issues with the GNO chain (I could only test with the prod CGW tho)

@francovenica
Copy link
Contributor

Besides the previous comment I was able to use the relayer in the GNO chain pretty well.

The only thing notisable was when I tried to upgrade the safe. I had the warning that the tx could fail, but that error might be that old issue about the GNO chain underestimating how much gas and gas fee it needs to execute these types of tx. I executed the tx anyways with the relayer and it worked fine
image

@iamacook
Copy link
Member Author

iamacook commented Apr 5, 2023

With the stg CGW I cannot create safes in goerli With the stg or prod CGW I cannot execute tx with the tx builder app in goerli

Our 1Balance pool was empty and transactions could not be relayed. If you encounter transactions not relaying, be sure to check the network tab for Gelato requests.

We have since topped up the pool and transactions are succeeding. I have managed to create Safes/execute transactions successfully.

The only thing notisable was when I tried to upgrade the safe. I had the warning that the tx could fail, but that error might be that old issue about the GNO chain underestimating how much gas and gas fee it needs to execute these types of tx. I executed the tx anyways with the relayer and it worked fine

There was an issue with the Gnosis Chain RPC yesterday which may have played a role. If the relay succeeded, it leads me to believe that Gelato are likely using a different RPC to us.

I checked this myself and was not shown a warning:

image

Could you try the above again today and let me know if you continue to experience issues?

@francovenica
Copy link
Contributor

I've checked Goerli again and now it works. I didn't thought about the balance pool since I was able to execute normal tx (but those are like a 1/10th of what a safe creation cost, so maybe there was just enough for regular tx's 🤷 )

I tried The safe update and I still see the warning that the tx might fail, but I just bumped the gas up in the adv parameters and the warning is gone, so is not a relayer thing.

@iamacook
Copy link
Member Author

iamacook commented Apr 5, 2023

I tried The safe update and I still see the warning that the tx might fail, but I just bumped the gas up in the adv parameters and the warning is gone, so is not a relayer thing.

We've noticed RPC issues again today. Taking that into consideration, are you ok if we merge this?

@francovenica
Copy link
Contributor

Yes, merge it

@iamacook iamacook merged commit c41d622 into dev Apr 5, 2023
@iamacook iamacook deleted the remove-getweb3 branch April 5, 2023 13:21
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signing a message from a DApp crashes the app
3 participants