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

fix: web3 transactions & readonly methods #3447

Merged
merged 7 commits into from
Feb 28, 2025
Merged

fix: web3 transactions & readonly methods #3447

merged 7 commits into from
Feb 28, 2025

Conversation

lorux0
Copy link
Collaborator

@lorux0 lorux0 commented Feb 26, 2025

Pull Request Description

What does this PR change?

Fixes #3186

There were many problems when scenes were asking for sendAsync to retrieve info or attempt to do any transaction from the blockchain.
First of all the flow was broken because it was trying to connect the web socket (to auth api) on a wrong threading context. It has been fixed by switching to main thread. We also had a concurrency problem. If the scene was requesting many operations in a row it was getting messed up as it was not thread safe. To fix this a mutex has been introduced so we only allow one sendAsync operation at a given time.
Secondly, the dcl-crypto-toolkit is performing many eth_calls which was provoking the explorer to open many browser tabs to allow operations with the wallet. This was wrong as we should not ask the user to confirm on the auth site for every operation. The fix consists on identifying a set of ethereum methods that are considered readonly, followed by a new flow that connects to the decentraland rpc server that fullfulls the data requested from the blockchain. Only transactions will require user signature in the auth site.
Finally the dcl-crypto-toolkit requires that the user's profile hasConnectedWeb3 field set to true. Catalysts do not manipulate this field, so it has been forced during the auth screen as we asume that all users that follow the authentication flow are already connected to web3.

Test Instructions

There is no scene that uses this functionality in production. I can confirm that works during my tests with a local scene. I will ask someone from sdk to verify that its working fine.

Test Steps

  1. Test the login flow. Either change account or use the one that you already have. Everything should keep working as expected
  2. Do a roundup in the world and check everything is fine overall

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

Code Review Reference

Please review our Code Review Standards before submitting.

@lorux0 lorux0 requested review from a team as code owners February 26, 2025 19:18
Copy link

@Ludmilafantaniella Ludmilafantaniella left a comment

Choose a reason for hiding this comment

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

Approved! ✅

Tested on both Windows and Mac. Everything works fine using new scenes, old Metamask accounts, and Social Accounts.

I couldn't verify ticket #3186 because the method I usually use with local-scene wasn't working (it wouldn't open). Let me know if there's another way to check it!

3447EvidenciaAp.mp4

Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:

  • ✔️ Log In/Log Out
  • ✔️ Backpack and wearables in world
  • ✔️ Emotes in world and in backpack
  • ✔️ Teleport with map/coordinates/Jump In
  • ✔️ Chat and multiplayer
  • ✔️ Profile card
  • ✔️ Camera
  • ✔️ Skybox

@lorux0 lorux0 requested review from pravusjif and removed request for popuz and mikhail-dcl February 27, 2025 14:18
Copy link
Member

@pravusjif pravusjif left a comment

Choose a reason for hiding this comment

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

Great job, LGTM!

@lorux0 lorux0 merged commit ca1d675 into dev Feb 28, 2025
5 checks passed
@lorux0 lorux0 deleted the fix/web3-transactions branch February 28, 2025 03:06
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.

E@ scenes don't listen to Metamask
3 participants