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: Extension notifications should show the username when using metatransactions #4220

Conversation

iamsamgibbs
Copy link
Contributor

@iamsamgibbs iamsamgibbs commented Jan 31, 2025

Description

See block-ingestor PR

This PR makes a change to how the creator name is set for extension related actions.

Previously the block-ingestor used receipt.from as the creator for all extension related actions as the extension event does not emit the msg_sender. This works fine when metatransactions are disabled, but when they are enabled the metatransaction address in place of the original creator.

This PR adds a new getTransactionSignerAddress function to the block-ingestor (courtesy of @bassgeta 's implementation on the proxy colony PRs!) and replaces uses of receipt.from to get the address of the user who actually installed the extension.

This means the correct user now shows in both notifications and the extension details.

Warning

If anyone knows of a reason why replacing the metatransactions address with the user address is a bad idea, please let me know!

Testing

  • Step 1: Run the dev environment with the notifications flag to enable notifications npm run dev --notifications
  • Step 2: Login with dev wallet 1 and enable metatransactions in the user profile

Screenshot 2025-01-31 at 10 42 57

  • Step 3: Install an extension and check the notifications.

Screenshot 2025-01-31 at 10 43 24

Screenshot 2025-01-31 at 10 43 30

Diffs

New stuff

  • getTransactionSignerAddress function added

Changes 🏗

  • receipt.from replaced with getTransactionSignerAddress

Resolves #3913

@iamsamgibbs iamsamgibbs self-assigned this Jan 31, 2025
@iamsamgibbs iamsamgibbs marked this pull request as ready for review January 31, 2025 10:51
@iamsamgibbs iamsamgibbs requested a review from a team as a code owner January 31, 2025 10:51
@iamsamgibbs iamsamgibbs force-pushed the fix/3913-extension-transactions-should-use-correct-creator branch from ffa7dc4 to 2928bcd Compare January 31, 2025 10:55
bassgeta
bassgeta previously approved these changes Jan 31, 2025
Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Glad to see that helper solve this issue! Nice spot and a nice solution 💯
image
image

mmioana
mmioana previously approved these changes Feb 3, 2025
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Really nice work @iamsamgibbs 🎉

Now the notifications properly show the username when using metatransactions 💯
Enabled metatransactions
Screenshot 2025-02-03 at 11 50 53
Installed the extension, deprecated it, uninstalled it, installed it again and changed the threshold
Screenshot 2025-02-03 at 11 51 12
And all notifications contained the username 💯
Screenshot 2025-02-03 at 11 52 45

davecreaser
davecreaser previously approved these changes Feb 3, 2025
Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

Nice one @iamsamgibbs thanks for fixing this 📈

Notifications and the extension page correctly show Leela now:

Screenshot 2025-02-03 at 12 45 43 Screenshot 2025-02-03 at 12 46 23 Screenshot 2025-02-03 at 12 46 38 Screenshot 2025-02-03 at 12 46 57 Screenshot 2025-02-03 at 12 47 22

@iamsamgibbs iamsamgibbs dismissed stale reviews from davecreaser, mmioana, and bassgeta via a9df1c5 February 4, 2025 10:03
@iamsamgibbs iamsamgibbs force-pushed the fix/3913-extension-transactions-should-use-correct-creator branch from 2928bcd to a9df1c5 Compare February 4, 2025 10:03
@iamsamgibbs iamsamgibbs force-pushed the fix/3913-extension-transactions-should-use-correct-creator branch from a9df1c5 to ae66f6d Compare February 4, 2025 10:04
Copy link
Contributor

@davecreaser davecreaser left a comment

Choose a reason for hiding this comment

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

That's a beautiful new hash right there!

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

A much nicer txHash this time 😎
Reapproving after rebase, let's go!

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Re-approving the hash change 👍

@iamsamgibbs iamsamgibbs merged commit f41fd90 into master Feb 4, 2025
2 checks passed
@iamsamgibbs iamsamgibbs deleted the fix/3913-extension-transactions-should-use-correct-creator branch February 4, 2025 11:33
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.

[PROD] Notifications: Extension enabling not picking up username
4 participants