Skip to content
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

fix: update argent mobile rpc methods #104

Merged
merged 5 commits into from
Jul 3, 2024
Merged

Conversation

bluecco
Copy link
Contributor

@bluecco bluecco commented Jun 4, 2024

No description provided.

@dhruvkelawala
Copy link
Contributor

@mluisbrown Plan is to update mobile-bridge PR once get-starknet has a stable release, i.e everything is merged on master branch. Currently it's on develop

@bluecco
Copy link
Contributor Author

bluecco commented Jun 4, 2024

@mluisbrown let's keep this pr open until we are all on the same page

@bluecco bluecco marked this pull request as draft June 13, 2024 15:06
@bluecco bluecco added the DON'T MERGE No merge till final decision label Jun 13, 2024
@@ -50,9 +50,9 @@ export class StarknetAdapter
// NamespaceAdapter
public namespace = "starknet"
public methods = [

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to be backwards compatible, so to the outside world we should expose wallet_* methods, but to the mobile client via WC starknet_* requests should be send

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI on the cloient side we will add support for wallet_ prefix as well, so that we can get rid of the starknet_ prefix at some point in the future cc: @mluisbrown

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kamilargent just to be clear
atm we'll accept wallet_ and, in starknetkit, rename that to starknet_ for this current release?

Copy link

@kamilargent kamilargent Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to the Dapp developer it should look like we use wallet_requestAddInvokeTransaction but internally to WC everything with starknet_ will be send. And it will have to be kept like that for a few months, until we get nice adoption of mobile apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kamilargent updated

@bluecco bluecco marked this pull request as ready for review July 3, 2024 10:17
@bluecco bluecco removed the DON'T MERGE No merge till final decision label Jul 3, 2024
const requestToCall = this.handleRequest[call.type]
let type = call.type as string

if (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: maybe worth adding a comment that we are adding it temporarily for backwards compatibility and mobile will add support for starknet_* calls that we can switch to in few months

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yep

Copy link

@kamilargent kamilargent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let us know when the test dapp will be updated so that we can confirm it's all good

@bluecco bluecco merged commit 07dd240 into develop Jul 3, 2024
1 check passed
@bluecco bluecco deleted the fix/snjs-argent-mobile branch July 3, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants