MIPX v1 - New json rpc method for swaps #25

tommasini · 2023-11-10T22:41:12Z

MIP for new json rpc method wallet_swapAsset

vandan · 2023-11-11T00:00:45Z

Probably worth referencing the related EIP as part of the MIP: https://github.com/tommasini/EIPs/blob/64090fe54761906fc9bde31ad46eb85230de5e8d/EIPS/eip-swap-rpc.md

tommasini · 2023-11-11T00:08:08Z

Thanks for the review @vandan
Added!

vandan · 2023-11-11T00:40:01Z

Just sharing some initial thoughts:

Is there anything that should be different or more explicit when a swap operation is triggered from a dapp rather than manually in the wallet UI?
I noticed that the request params are abstracted away from specific accounts. If a dapp populates swap values in the swap form, but the user wants to run the swap off of a different account address, then the UI should probably attempt to retain the other swap parameters (when possible). As an example, MetaMask mobile doesn't appear to support this behavior right now.
You can add a section for User Experience Considerations to add any UI guidelines that relate to this MIP.

Can you add more detail to the Motivation section? It would help other developers to understand when and why they should use this method.
Try to explain why this proposal is necessary and what impact it would have for the developer ecosystem. What problem does it solve (helping users get tokens they may need for a specific operation with a dapp, ensuring that users get the right tokens in their swaps, etc.)? What alternatives were considered (ex. extending via a Snap)? Why is this proposal preferred over the alternatives?

I'm also curious about what transparency should be provided when something goes wrong. What errors might be returned to the requesting dapp?

vandan · 2023-11-11T00:42:50Z

Please set this MIP as Type: Community

MIPs/mip-2.md

vandan · 2023-11-15T23:31:30Z

I assume this method would be limited to accepting ERC-20 tokens, right? Is there anything that should be considered for future flexibility?
We might want to consider making the method name a little more specific to something like: wallet_swapToken or wallet_swapAsset

MIPs/mip-2.md

shanejonas · 2023-11-16T14:56:12Z

I assume this method would be limited to accepting ERC-20 tokens, right? Is there anything that should be considered for future flexibility? We might want to consider making the method name a little more specific to something like: wallet_swapToken or wallet_swapAsset

I do like wallet_swapAsset to be more in-line with wallet_watchAsset that supports multiple asset types now.

Co-authored-by: Vandan <[email protected]>

tommasini · 2023-11-27T13:03:24Z

@vandan @shanejonas
Thanks for the review and amazing point on the account not being on the params!

Shane Jonas suggestion makes a lot of sense! We can use the same logic as we use on the send transaction rpc method, and use the account connected to the dapp, adding the from field that way!

In the other hand it would be nice to use the current screens with the swaps logic that already exist, what do you think if we ask the user to switch the selected account to account to the new from address field (if it's different than the one selected) instead of creating a new screen with all the swaps logic? (We can also do the first implementation since it's quicker and re-uses logic, and improve it to the second one if we see a big adoption, in order to improve it)

Curious, what do you think it's the best name for the property, from_address, user_address or just address?

Can you add more detail to the Motivation section? It would help other developers to understand when and why they should use this method.

I will add! Thanks!

I'm also curious about what transparency should be provided when something goes wrong. What errors might be returned to the requesting dapp?
In terms of error handling to the dapp right now we validate the params, we send a message back when some property is wrong on these formats:

from is not defined
to is not defined
amount property of from is not defined
chainId property of from is not defined
token_address property of from is not defined
token_address property of to is not defined
chainId property of from is not defined

tommasini · 2023-11-27T13:15:00Z

wallet_swapAsset It's sounds amazing to me as well!!

tommasini · 2023-11-27T13:29:28Z

Updated motivation section

MIPs/mip-3.md

tommasini · 2023-11-28T16:37:10Z

I assume this method would be limited to accepting ERC-20 tokens, right? Is there anything that should be considered for future flexibility?

@vandan When you say ERC-20 tokens, are you excluding BEP-20 and main tokens? I don't see a reason why limit it to ERC-20 tokens

vandan · 2023-11-29T22:53:01Z

Random thought since I'm hearing more about cross-chain DEXs recently.
Should we at least think about the implications for how non-EVM tokens might be accommodated in a wallet swap API. Just in case there is something we can do to keep it flexible enough for something like that in the future.

vandan · 2023-11-29T22:54:22Z

I assume this method would be limited to accepting ERC-20 tokens, right? Is there anything that should be considered for future flexibility?

@vandan When you say ERC-20 tokens, are you excluding BEP-20 and main tokens? I don't see a reason why limit it to ERC-20 tokens

I was just thinking about that as well. Planning for some flexibility beyond ERC-20s makes sense to me as well.

vandan · 2023-12-06T05:38:07Z

A potentially relevant proposal around chain identifiers from ChainAgnostic: https://github.com/ChainAgnostic/CAIPs/blob/main/CAIPs/caip-2.md

…am to represent the user wallet address

… the wallet is not required

rekmarks · 2024-06-10T15:45:20Z

Could this be made to work in a multichain setting (i.e. using CAIP addresses), or Ethereum only?

tommasini · 2024-06-12T16:36:10Z

@rekmarks Yes, it's totally be possible to be prepared for that!
It would be something like this structure

We could have it on the MIP as a future implementation, since on the mobile app the UI is not prepared for it yet, but when the multichain is implemented we can add this logic without breaking the default behaviour (that uses the network conected to the dapp as the default)

ethereum.request({
  method: 'wallet_swapAsset',
  params: [{
    fromToken: [{
      address: '0x1234567890abcdefABCDEF1234567890ABCDEF',
      value: '0xDE0B6B3A7640000',
      chainId: // CAIP-10 or eth
    }],
    toToken: {
      address: '0xabcdef1234567890ABCDEF1234567890abcdef',
       chainId: // CAIP-10 or eth
    },
    userAddress: '0x0000000000000000000000000000000000000000'
  },
  ]
});

Nevertheless, this was discussed and we talked about making this json rpc because the purpose would be for in first implementation to only redirect to the swaps flow already implemented on the wallet, and not with bridging functionality associated to ship it quicker and test the adoption of it! We were thinking about shipping other json rpc method for bridging/ cross chain swaps when this ones were implemented and the adoption of the builders community was validated. Happy to know your thoughts around this!

rekmarks · 2024-06-12T16:48:26Z

Thanks @tommasini, that sounds good. I mainly wanted to make sure that you guys had multichain in mind.

Some notes about your proposed format, just for future reference:

The chainId field would be CAIP-2 strings, not CAIP-10.
Rather than having separate chainId fields, the address fields could just be CAIP-10 addresses, which are prefixed with the CAIP-2 chain id.
- If we do have separate chainId and address fields, the addresses should probably be in the format of the chain in question, since CAIP-10 is simply <caip2>:<address>.
- This is part of a larger question of "how do we handle CAIP addresses in our codebase?". Personally, I think we should establish sophisticated CAIP string parsing utilities, and go with the CAIP-10 approach.
The chain of the userAddress should also be specified, either by converting the field to CAIP-10 or by adding a chainId field.

MIPs/mip-x.md

jiexi · 2024-06-21T19:26:27Z

MIPs/mip-x.md


  ```markdown
  if (parseChainIdFromCaip10(fromToken.address) !== parseChainIdFromCaip10(toToken.address)) {
-  throw rpcErrors.invalidParams(
+  throw rpcErrors.methoddNotSupported(


is the double d in methodd on purpose? confused since i see you made this change in two different places haha

AHAHAHAHHA and I missed this error on two diff places XD

vandan

This MIP generally looks good to me for committing as Status: Draft.

Co-authored-by: jiexi <[email protected]>

MIPs/mip-x.md

adonesky1 · 2024-06-25T17:59:41Z

MIPs/mip-x.md

+throw rpcErrors.invalidParams('Invalid caip-10 user address');
+}
+
+try {
+parsedCaip10FromTokenAddress = parseCaip10Address(
+fromToken[0].address,
+);
+} catch (error) {
+throw rpcErrors.invalidParams('Invalid caip-10 fromToken address');
+}
+
+try {
+parsedCaip10ToTokenAddress = parseCaip10Address(toToken.address);
+} catch (error) {
+throw rpcErrors.invalidParams('Invalid caip-10 toToken address');


Maybe same here: caip-10 -> CAIP-10? I know these are the actual error messages so maybe not...?

I think this section would be clearer in english instead of a try catch code example

adonesky1

Just a few non blocking comments/requests! Ready for draft status for sure!

MIPs/mip-x.md

Co-authored-by: jiexi <[email protected]>

tommasini added 4 commits November 10, 2023 22:40

mip2 v1

1447a80

Remove non used sections

c937f19

fix typo

4d007d1

change example amount to 1 token value

2487b5c

tommasini mentioned this pull request Nov 10, 2023

feat: New json rpc for swaps MetaMask/metamask-mobile#7509

Draft

14 tasks

added reference for draft EIP

bf9e4d2

vandan reviewed Nov 11, 2023

View reviewed changes

MIPs/mip-2.md Outdated Show resolved Hide resolved

shanejonas reviewed Nov 16, 2023

View reviewed changes

MIPs/mip-2.md Outdated Show resolved Hide resolved

Update MIPs/mip-2.md

c8ec086

Co-authored-by: Vandan <[email protected]>

tommasini added 2 commits November 27, 2023 13:28

changed for MIP-3 and updated Motivation Section

7b65338

deleted mip2

cb3717d

shanejonas reviewed Nov 28, 2023

View reviewed changes

MIPs/mip-3.md Outdated Show resolved Hide resolved

update motivatino

59b0df9

tommasini changed the title ~~MIP2 v1 - New json rpc method for swaps~~ MIP3 v1 - New json rpc method for swaps Nov 29, 2023

tommasini added 3 commits December 20, 2023 12:50

add comparison to snaps to the motivation section

a0ba3ae

change params to use position params instead and add user_address par…

89c269a

…am to represent the user wallet address

add user_address property to the json rpc method

ed3b859

tommasini added 4 commits May 30, 2024 15:12

added dash on json rpc appearances, renamed mip to x

55c71d3

address review for copy

d64432f

added explanation while checking the selected account on the dapp and…

e3f7244

… the wallet is not required

address all comments on the review of the mip

2b1211a

tommasini changed the title ~~MIP3 v1 - New json rpc method for swaps~~ MIPX v1 - New json rpc method for swaps Jun 10, 2024

tommasini added 2 commits June 21, 2024 14:05

change addresses to use caip10 format

3fe0f34

update cross chain swap error method

c119eeb

jiexi reviewed Jun 21, 2024

View reviewed changes

MIPs/mip-x.md Outdated Show resolved Hide resolved

jiexi reviewed Jun 21, 2024

View reviewed changes

added caip-10 error handling

f78e3ad

vandan previously approved these changes Jun 24, 2024

View reviewed changes

Update MIPs/mip-x.md

09fd777

Co-authored-by: jiexi <[email protected]>

tommasini dismissed vandan’s stale review via 09fd777 June 25, 2024 13:29

removing the double d from method not supported

326c0a4

adonesky1 reviewed Jun 25, 2024

View reviewed changes

MIPs/mip-x.md Outdated Show resolved Hide resolved

adonesky1 reviewed Jun 25, 2024

View reviewed changes

MIPs/mip-x.md Outdated Show resolved Hide resolved

adonesky1 reviewed Jun 25, 2024

View reviewed changes

MIPs/mip-x.md Outdated Show resolved Hide resolved

adonesky1 reviewed Jun 25, 2024

View reviewed changes

adonesky1 previously approved these changes Jun 25, 2024

View reviewed changes

jiexi reviewed Jun 25, 2024

View reviewed changes

MIPs/mip-x.md Show resolved Hide resolved

Update MIPs/mip-x.md

61f515e

Co-authored-by: jiexi <[email protected]>

adonesky1 dismissed their stale review via 61f515e July 1, 2024 15:26

adonesky1 added 3 commits July 1, 2024 09:26

Update MIPs/mip-x.md

5892b6b

Update MIPs/mip-x.md

cae24e9

Update MIPs/mip-x.md

a4c38d4

adonesky1 approved these changes Jul 1, 2024

View reviewed changes

adonesky1 merged commit 1c60c00 into MetaMask:main Jul 1, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIPX v1 - New json rpc method for swaps #25

MIPX v1 - New json rpc method for swaps #25

tommasini commented Nov 10, 2023 •

edited

Loading

vandan commented Nov 11, 2023

tommasini commented Nov 11, 2023

vandan commented Nov 11, 2023

vandan commented Nov 11, 2023

vandan commented Nov 15, 2023 •

edited

Loading

shanejonas commented Nov 16, 2023

tommasini commented Nov 27, 2023 •

edited

Loading

tommasini commented Nov 27, 2023

tommasini commented Nov 27, 2023

tommasini commented Nov 28, 2023

vandan commented Nov 29, 2023

vandan commented Nov 29, 2023

vandan commented Dec 6, 2023

rekmarks commented Jun 10, 2024 •

edited

Loading

tommasini commented Jun 12, 2024

rekmarks commented Jun 12, 2024

jiexi Jun 21, 2024

tommasini Jun 25, 2024

vandan left a comment

adonesky1 Jun 25, 2024

jiexi Jun 25, 2024

adonesky1 left a comment

MIPX v1 - New json rpc method for swaps #25

MIPX v1 - New json rpc method for swaps #25

Conversation

tommasini commented Nov 10, 2023 • edited Loading

vandan commented Nov 11, 2023

tommasini commented Nov 11, 2023

vandan commented Nov 11, 2023

vandan commented Nov 11, 2023

vandan commented Nov 15, 2023 • edited Loading

shanejonas commented Nov 16, 2023

tommasini commented Nov 27, 2023 • edited Loading

tommasini commented Nov 27, 2023

tommasini commented Nov 27, 2023

tommasini commented Nov 28, 2023

vandan commented Nov 29, 2023

vandan commented Nov 29, 2023

vandan commented Dec 6, 2023

rekmarks commented Jun 10, 2024 • edited Loading

tommasini commented Jun 12, 2024

rekmarks commented Jun 12, 2024

jiexi Jun 21, 2024

Choose a reason for hiding this comment

tommasini Jun 25, 2024

Choose a reason for hiding this comment

vandan left a comment

Choose a reason for hiding this comment

adonesky1 Jun 25, 2024

Choose a reason for hiding this comment

jiexi Jun 25, 2024

Choose a reason for hiding this comment

adonesky1 left a comment

Choose a reason for hiding this comment

tommasini commented Nov 10, 2023 •

edited

Loading

vandan commented Nov 15, 2023 •

edited

Loading

tommasini commented Nov 27, 2023 •

edited

Loading

rekmarks commented Jun 10, 2024 •

edited

Loading