feat: added new method to set owner after establishing connection signer with wallet - #504
feat: added new method to set owner after establishing connection signer with wallet#504roman-nazaruk wants to merge 6 commits into
Conversation
| }); | ||
| }; | ||
|
|
||
| export const notifyErrorNotInitializes = (notify: Notify): void => { |
There was a problem hiding this comment.
Please provide this in another PR.
| }); | ||
|
|
||
| export const SignerOptionsSchema = z.object({ | ||
| export const SignerInitOptionsSchema = z.object({ |
There was a problem hiding this comment.
Why are you creating a duplicate of the options?
Why are host and sessionOptions provided in both SignerInitOptionsSchema and SignerOptionsSchema?
If this is necessary, can at least parts of it be provided in a separate PR with related tests?
| private handleOwnerInitialized({data: msgData, origin}: SignerMessageEvent): { | ||
| initialized: boolean; | ||
| } { | ||
| if (isNullish(this.#owner)) { |
There was a problem hiding this comment.
Should we use Zod to assert not nullish and no anonymous as well? Just in case sh** happens.
| return {handled: false}; | ||
| } | ||
|
|
||
| if (isNullish(this.#owner)) { |
There was a problem hiding this comment.
This does not sound correct, should an error be throw somewhere if the owner is null at this point?
|
|
||
| const currentPermission = sessionScopeState({ | ||
| owner: owner.getPrincipal(), | ||
| owner: this.#owner?.getPrincipal(), |
There was a problem hiding this comment.
optional chaining is not necessary here no?
| window.addEventListener('message', this.onMessageListener); | ||
| } | ||
|
|
||
| setOwner = (owner: IdentityNotAnonymous) => { |
There was a problem hiding this comment.
-
Can you add the missing JS docs.
-
Should we use an object as parameter? {owner}: {owner: IdentityNotAnonymous}?
| }); | ||
| }); | ||
|
|
||
| describe('Should init Signer without owner and notify error regarding owner not set', () => { |
There was a problem hiding this comment.
Can we perform the same test for every types of request not solely for status?
| setOwner = (owner: IdentityNotAnonymous) => { | ||
| IdentityNotAnonymousSchema.parse(owner); | ||
|
|
||
| this.#owner = owner; |
There was a problem hiding this comment.
Shouldln't we add an assertion that disallow overwriting the owner once set?
| disconnect = (): void => { | ||
| window.removeEventListener('message', this.onMessageListener); | ||
| this.#walletOrigin = null; | ||
| this.#owner = null; |
There was a problem hiding this comment.
Is there a test that asserts the owner is set back to null after disconnect?
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it('should emit NOT_INITIALIZED error when a status message is received and no owner is set', async () => { |
There was a problem hiding this comment.
Isn't this test actually incorrect? Status and supported standard should be emitted even if the signer is not initialized. Can you improve the code and add some tests in that direction?
2 participants
Motivation
We are updating the oisy-signer so that it now waits for a valid owner to be set before processing non‑read‑only messages, ensuring that it returns a clear NOT_INITIALIZED error when not logged in.
Changes
Added new method to init owner after the connection was established between Signer and wallet
Tests
Added new unit test to cover new logic.