Initial Mobile support (ussing cip45) #179
Conversation
WalkthroughThe updates revolve around the integration and testing of the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
hrajchert
force-pushed
the
hrajchert/cip45
branch
from
54a4433 to
22df930
Compare
Co-authored-by: Brian W Bush <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
packages/wallet/package.jsonis excluded by:!**/*.json
Files selected for processing (8)
- changelog.d/20240112_130130_hrajchert_plt_9089_createContract_bundle_support.md (1 hunks)
- changelog.d/20240122_133315_hrajchert_cip45.md (1 hunks)
- examples/cip45-flow/Readme.md (1 hunks)
- examples/cip45-flow/index.html (1 hunks)
- examples/cip45-flow/index.js (1 hunks)
- jsdelivr-npm-importmap.js (1 hunks)
- packages/wallet/src/browser/index.ts (2 hunks)
- packages/wallet/src/peer-connect/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- changelog.d/20240112_130130_hrajchert_plt_9089_createContract_bundle_support.md
Files skipped from review as they are similar to previous changes (6)
- examples/cip45-flow/Readme.md
- examples/cip45-flow/index.html
- examples/cip45-flow/index.js
- jsdelivr-npm-importmap.js
- packages/wallet/src/browser/index.ts
- packages/wallet/src/peer-connect/index.ts
Additional comments: 1
changelog.d/20240122_133315_hrajchert_cip45.md (1)
- 4-4: The changelog entry is clear and informative, providing necessary details about the new feature and its associated PR.
| import * as Browser from "../browser/index.js"; | ||
|
|
||
| type WalletHandler = (walletId: string, wallet: WalletAPI) => void; | ||
| export const mkPeerConnectAdapter = () => { |
There was a problem hiding this comment.
this is hard to understand
There was a problem hiding this comment.
I can add some documentation around it. Have you seen the example to clarify on use?
There was a problem hiding this comment.
or is there anything in particular that you would need clarification?
There was a problem hiding this comment.
I would have wrapped this function directly in that file :
const dAppConnect = new CardanoPeerConnect.DAppPeerConnect({
dAppInfo: dAppInfo,
verifyConnection: verifyConnection,
onApiInject: adapter.adaptApiInject,
onApiEject: adapter.adaptApiEject,
});
And provide explanation about how to create and use this DAppPeerConnect function. I would have kept adapter.adaptApiInject and adapter.adaptApiEject private into this module also
There was a problem hiding this comment.
Nevertheless, it is good enough like that imo... I would focus on WalletConnect and merge this as it is...
| getTokens: Browser.getTokens(di), | ||
| getLovelaces: Browser.getLovelaces(di), | ||
| }; | ||
| wallet = peerWallet; |
There was a problem hiding this comment.
why not using this function instead ?
/**
* Returns an instance of the browser wallet API for the specified wallet.
* @param walletName - The name of the wallet to get an instance of.
* @returns An instance of the BrowserWalletAPI class.
*/
export async function mkBrowserWallet(
walletName: SupportedWalletName
): Promise<WalletAPI> {
if (
getInstalledWalletExtensions()
.map((extension) => extension.name.toLowerCase())
.includes(walletName.toLowerCase())
) {
const extension = await window.cardano[walletName.toLowerCase()].enable();
const di = { extension };
return {
waitConfirmation: waitConfirmation(di),
signTx: signTx(di),
getChangeAddress: getChangeAddress(di),
getUsedAddresses: getUsedAddresses(di),
getCollaterals: getCollaterals(di),
getUTxOs: getUTxOs(di),
isMainnet: isMainnet(di),
getTokens: getTokens(di),
getLovelaces: getLovelaces(di),
};
} else {
throw new Error(`Wallet ${walletName} is not available in the browser`);
}
}
There was a problem hiding this comment.
and why exposing all the internal functions ?
There was a problem hiding this comment.
So, after you do the handshake, cardano peer connect offers a CIP-30 compatible API and I needed an adaptor from CIP-30 to Marlowe's WalletAPI.
This is not a browser wallet, so using that function would be confusing. I also did not wanted to change getInstalledWalletExtensions for the same reason.
Note that the actor in charge of creating the wallet is inverted from normal uses. We get a callback when somebody connects rather than the dapp being able to create a wallet connection at will.
Instead of exposing the inner functions we could create an adaptCIP30Wallet and use that instead.
If we want to add support for the walletConnect standard through adalib we can also use the same adaptor
Summary
This PR adds initial Mobile support to the Marlowe TS-SDK by providing an adaptor module to the cardano-peer-connect library. You can follow the library instructions to configure a Dapp and use the new
@marlowe.io/wallet/peer-connectadaptor to provide theonApiInjectandonApiEjectmethods.How to test
An example is provided in the
examples/cip45-flowfolderCurrent limitations found:
DAppPeerConnectclass. So the wallet can disconnect from the dapp but the dapp can't easily disconnect from the wallet. See Issue 57Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Refactor
Style