-
Notifications
You must be signed in to change notification settings - Fork 34
sapphire-wagmi-v2: Add EIP-6963 support #550
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
Conversation
✅ Deploy Preview for oasisprotocol-sapphire-paratime canceled.
|
183085f
to
401322a
Compare
720f252
to
2cd6377
Compare
Playwright test seems to be failing, but I could only reproduce it once. Needs investigation, seems there is an alert that need to be dismissed upon deploying the contract. |
Playwright seems flaky. Also failed on @ZigaMr PR https://github.com/oasisprotocol/sapphire-paratime/actions/runs/14178490717/job/39718975456?pr=552 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interface seems reasonable to me.
Not introduced in this PR, but I find it weird that sapphire-wagmi-v2
whitespace is so different from sapphire-paratime
, and prefer the denser/smaller one.
The issue here is that |
const _addEventListener = EventTarget.prototype.addEventListener; | ||
Object.defineProperty(EventTarget.prototype, "addEventListener", { | ||
value: function ( | ||
type: string, | ||
callback: EventListenerOrEventListenerObject | null, | ||
options?: AddEventListenerOptions | boolean, | ||
): void { | ||
if (type === EIP6963_ANNOUNCE_PROVIDER_EVENT_NAME) { | ||
_addEventListener.call( | ||
this, | ||
type, | ||
(event: Event) => { | ||
let patchCustomEvent = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it breaks removeEventListener(EIP6963_ANNOUNCE_PROVIDER_EVENT_NAME, callback, options) because callback function won't match inline patch function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If removing EIP6963_ANNOUNCE_PROVIDER_EVENT_NAME listeners isn't common then I would just add a headsup comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen a codebase, that would try to remove the event. But yeah, I see the potential issue. Thats why I didn't add "restore" to original _addEventListener
, which could cause memory leaks, because I did not expect that someone would try to remove the event listener.
Either way, I added the comment here: #565. I hope that is enough for now, and if there is an use-case for it. I am happy to implement more robust solution, that would "reverse" event patching.
Closes #499