Fix for Event Name Collisions #194
Conversation
piotrwitek
requested review from
vladimir-gachkovsky
and removed request for
mssteuer,
hoffmannjan and
mrkara
There was a problem hiding this comment.
looks good to me, let's also get a review from @vladimir-gachkovsky on Monday, and then we can merge.
|
@vladimir-gachkovsky on Monday let's have a call and I'll let you know how to test the event name collision issue. |
src/background/SigningManager.ts
Outdated
| export default class SigningManager extends events.EventEmitter { | ||
| private unsignedDeploys: deployWithID[]; | ||
| private unsignedMessages: messageWithID[]; | ||
| private nextId: number; | ||
| private popupManager: PopupManager; | ||
| private messagePrefix: string = `casper-signer`; | ||
| private signingFinished: string = `finished`; |
There was a problem hiding this comment.
@George-cl signingFinished is not clear, shouldn't this be named messageSuffix? It is used in the same pattern as the casper-signer prefix and being at the end of the message signifies it is a suffix. Also, I don't see any other suffixes apart from finished, so what is the use case for using that at all?
There was a problem hiding this comment.
I've updated it to be messageSuffix. I think it's useful to prevent typos between where it's used in the emitter and the listener.
Although I guess we could remove that part of the event string entirely - is that what you're suggesting?
There was a problem hiding this comment.
I think we're good, we know it is working properly and it's properly documented as a suffix. Should be no problem in the future.
ghost
added this to the
This PR prefixes the events emitted by the
SigningManager.tswithcasper-signer:to reduce the risk of name collisions with other extensions.I was able to delegate stake on testnet with the Keplr extension installed and account created.
Asset
Signer for PR194