Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat:
externally_connectable
CAIP delivery and enveloping #25075feat:
externally_connectable
CAIP delivery and enveloping #25075Changes from 19 commits
e0c7edb
370dc06
c1ae3db
61e9697
1f8cfd2
5db9a97
6ef86a4
97469f9
1f58e7e
a9b741f
7df64f3
7db9d75
65c9d68
c8b66b5
bbb33ae
edab0fe
040d300
2acb719
9792594
7372604
b82888b
64ca655
714d044
29ea68c
64ba990
46bfee7
e24ef63
379ce8a
98ff148
cd3dd02
7f1eb8a
cbfd014
5d65634
3652ab3
a239e64
4f4f999
5b667f2
0c3f000
ab7767b
7fc0b23
ccd10ab
2eeac73
600daed
ee4dec5
dc6c189
d27526a
571cf9e
1ced8f3
487b9ca
323ab1e
7d3d5f6
2258ee7
3c8b832
439f27e
9f35203
fa34c32
c8ce2f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We've been trying to move away from passing around controller references as arguments, as we found it makes it easy to miss things when making breaking changes to controllers.
I see that this is pre-existing in this case though, so maybe we can clean this up later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the preference to pass a callback in instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. The preferences is to use the messenger, or callbacks for code not setup with a messenger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using the messenger where possible now ab7767b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe we could call this
connectExternalExtension
instead, to better reflect what it does. Even after we ship this, it won't really be a legacy feature.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess technically it was also used for the desktop connection, though that has since been removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good point. Also, we have an existing legacy stream already haha. 5b667f2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious to know what this refers to, i.e. what does it mean to "separate the legacy and multichain RPC pipelines", and how is this
requestAccountTabIds
state related?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gudahtt we are intending to create a separate JSON RPC pipeline for the new API (ticket here) for long term maintainability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gudahtt just want to confirm you've seen and understood this roadmap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the changes in this file need to be moved into the test-dapp. Only here for local testing/verification and to demonstrate how the CAIP Stream pipeline in the inpage provider is exactly the same one used in background
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Revert this before merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relevant poc test-dapp that enables externally_connectable without caip-x enveloping
https://github.com/MetaMask/test-dapp/pull/317/files