fulfill adr 006 #1567
fulfill adr 006 #1567
Conversation
🦋 Changeset detectedLatest commit: 3690624 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
turbocrime
force-pushed
the
turbocrime/fulfill-adr-006
branch
2 times, most recently
from
9e5a6e1 to
2651888
Compare
turbocrime
force-pushed
the
feat/adr-006-client-apis
branch
from
7f4d398 to
a19b911
Compare
turbocrime
force-pushed
the
turbocrime/fulfill-adr-006
branch
from
2651888 to
378152d
Compare
turbocrime
force-pushed
the
turbocrime/fulfill-adr-006
branch
from
378152d to
d8bbc2b
Compare
turbocrime
force-pushed
the
turbocrime/fulfill-adr-006
branch
from
027ab50 to
e336ca8
Compare
turbocrime
force-pushed
the
turbocrime/fulfill-adr-006
branch
from
e336ca8 to
87e821c
Compare
|
|
||
| await request; | ||
|
|
||
| return this.provider.manifest; |
There was a problem hiding this comment.
suggestion:
| return this.provider.manifest; | |
| return providerOrigin; |
Manifest URL usually appends /manifest to the origin but we want to keep returning the origin URL
There was a problem hiding this comment.
question: what's the point of removing react package?
| @@ -34,11 +35,12 @@ const handleErr = (e: unknown) => { | |||
|
|
|||
| const useExtConnector = () => { | |||
| const [result, setResult] = useState<boolean>(); | |||
| const navigate = useNavigate(); | |||
|
|
|||
| const request = async () => { | |||
| try { | |||
| await requestPraxAccess(); | |||
There was a problem hiding this comment.
issue: after the update, reloading Minifront results in displaying the connection screen.
suggestion: add isConnected() check and, if connected, navigate('/')
| /** Reexports the `isConnected` function from injected provider */ | ||
| readonly isConnected: () => boolean | undefined; |
There was a problem hiding this comment.
issue: please update this to
readonly isConnected: () => boolean| import './global.js'; | ||
|
|
||
| /** Return the specified provider, without verifying anything. */ | ||
| export const getPenumbraUnsafe = (penumbraOrigin: string) => |
There was a problem hiding this comment.
issue: not sure this naming makes sense if it can safely return undefined
|
|
||
| /** Return the specified provider after confirming presence of its manifest. */ | ||
| export const getPenumbra = (penumbraOrigin: string): Promise<PenumbraProvider> => | ||
| assertProviderManifest(penumbraOrigin).then(() => getPenumbraUnsafe(penumbraOrigin)!); |
There was a problem hiding this comment.
issue: instead of getPenumbraUnsafe(penumbraOrigin)!, think it best to throw an error if not present. Will help with debugging later when it does throw with a proper message.
| export interface IPenumbraClient { | ||
| /** | ||
| * Asks users to approve the connection to a specific browser manifest URL. | ||
| * If `manifest` argument is not provided, tries to connect to the first injected provider. |
There was a problem hiding this comment.
suggestion (idea):
| * If `manifest` argument is not provided, tries to connect to the first injected provider. | |
| * If `manifest` argument is not provided, tries to connect to the previously-connected or the first injected provider. |
from the implementation point, it's going to look over the isConnected() of all injected providers and connect to the first one with true
|
closing in favor of #1647 which will see more pushes with implementation of updated interface |
deletes react package fixes #1522 #1523
fixes #1521
prax-wallet/prax#120 will need work in prax but this interface is compatible
matching PR prax-wallet/prax#134
cc @VanishMax review
interfaces very close to meeting ADR as written.
isConnectedreturns a boolean onlycreatePenumbraClientcreateServiceClientis availableMinifront is updated to use
createPenumbraClientto init a transport. Larger refactor to supportcreateServiceClientis out of scope.