-
Notifications
You must be signed in to change notification settings - Fork 37
feat:wallet v1 (a.k.a Hub) #623
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
Conversation
bd7418c to
832116c
Compare
543f538 to
dbd0832
Compare
14dad39 to
69e9625
Compare
69e9625 to
6b107e7
Compare
| // Call connect on evm namespace of phantom provider | ||
| phantomProvider.get('evm').connect('0x1'); | ||
| ``` |
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.
Please add to document how could we connect to a provider. (all included namespaces)
| import { | ||
| legacyProviderImportsToVersionsInterface, | ||
| type Versions, | ||
| } from '@rango-dev/wallets-core/utils'; |
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 there any reason that this is not exported from wallet-core root? Also there are several similar cases if you search.
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.
importing using @rango-dev/wallets-core/utils is using ESM's exports feature which is what we use from now on. We only directly use @rango-dev/wallets-core whenever the environment doesn't support this feature. Currently, embedded is the only env that doesn't support for this. In future we will migrate that one as well since it needs some major changes and is out of scope of this task.
| } from '@rango-dev/wallets-shared'; | ||
| } from '@rango-dev/wallets-core/legacy'; | ||
| import type { BlockchainMeta, SignerFactory } from 'rango-types'; | ||
|
|
||
| import { Networks, WalletTypes } from '@rango-dev/wallets-shared'; | ||
| import { Networks } from '@rango-dev/wallets-core/legacy'; | ||
| import { WalletTypes } from '@rango-dev/wallets-shared'; |
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.
For some legacy providers like argentx, you've changed these imports but for most of them, you didn't. I think we should update all if there was a reason behind these changes.
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.
First I tried to remove Networks from shared but it affected many places, and in some places core wasn't available. I decided to keep Networks in shared but it actually is a re-export of wallets-core (wallets/shared/src/rango.ts:19).
So using import { Networks } from '@rango-dev/wallets-core/legacy' or import { Networks } from '@rango-dev/wallets-shared'; basically identical. To avoid put too much effort on changing this, I gave up on changing them in this PR. This change can be continue when we are migrating legacy providers to Hub.
If you think they shouldn't be changed, I can revert back import { Networks } from '@rango-dev/wallets-core/legacy'; to use shared as before.
widget/embedded/src/components/WalletNamespacesModal/WalletNamespacesModal.types.tsx
Outdated
Show resolved
Hide resolved
| export function evmPhantom() { | ||
| const instances = phantom(); | ||
|
|
||
| const evmInstance = instances?.get(Networks.ETHEREUM); |
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 think this produces a lot of duplicate code if we want to create a function for each provider-blockchain of each provider based on this approach. I think we should avoid this approach.
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.
You also used these functions in legacy code in v1 provider.
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 moved them to src/utils.ts since they will be used in Hub even legacy being 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.
I tried to make phantom, evmPhantom and solanaPhantom into one function but it doesn't improve the code. phantom() is using to detect wether the wallet is installed or not and also it's used in legacy implementation.
evmPhantom and solanaPhantom are using phantom to access to each namespace separately and throw an error if they are not available. this is because they are using inside hub's namespaces and they should throw error if they are not available. And also they are reusing in namespace, for example, evmPhantom is passing to changeAccountSubscriber and connect separetly.
By removing those function the provider-phantom/namespaces/evm.ts will look like this:
const [changeAccountSubscriber, changeAccountCleanup] =
actions.changeAccountSubscriber(() => {
const evmInstance = phantom()?.get(Networks.ETHEREUM) || undefined;
if (!evmInstance) {
throw new Error(
'Are you sure Phantom injected and you have enabled evm correctly?'
);
}
return evmInstance;
});
const evm = new NamespaceBuilder<EvmActions>('evm', WALLET_ID)
.action('init', () => {
console.log('[phantom]init called from evm cb');
})
.action([
...actions.recommended,
actions.connect(() => {
const evmInstance = phantom()?.get(Networks.ETHEREUM) || undefined;
if (!evmInstance) {
throw new Error(
'Are you sure Phantom injected and you have enabled evm correctly?'
);
}
return evmInstance;
}),
])
...| export function solanaPhantom() { | ||
| const instance = phantom(); | ||
| const solanaInstance = instance?.get(Networks.SOLANA); | ||
|
|
||
| if (!instance || !solanaInstance) { | ||
| throw new Error( | ||
| 'Are you sure Phantom injected and you have enabled solana correctly?' | ||
| ); | ||
| } | ||
|
|
||
| return solanaInstance; |
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.
almost duplicate as previous function.
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.
Let's keeping the discussion here: #623 (comment)
I will update this part as well if needed.
| utils.apply( | ||
| 'before', | ||
| [...before.recommended, ['connect', changeAccountSubscriber]], | ||
| evm | ||
| ); | ||
| utils.apply( | ||
| 'after', | ||
| [...after.recommended, ['disconnect', changeAccountCleanup]], | ||
| evm | ||
| ); |
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 think the logical way is to inject these methods using namespace builder (evm object) not using util.
| const evm = new NamespaceBuilder<EvmActions>('evm', WALLET_ID) | ||
| .action('init', () => { | ||
| console.log('[phantom]init called from evm cb'); | ||
| }) | ||
| .action([...actions.recommended, actions.connect(evmPhantom)]) | ||
| .andUse(and.recommended) | ||
| .build(); |
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 like the idea of using builders but the main problem for me is that if I want to write a new provider, I should go and find what we have in code or documentation and don't know what should be implemented.
We don't have a self-explanatory interface that asks us what needs to be implemented.
The other problem for me is that I can't understand what is this code doing without clicking on each function one by one to see their implementation details.
The other problem is that the life-cycle of provider and related hooks are ambiguous for me.
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.
As we've talked offline, each action can be imported from core and pass one by one using .action to builder. This is the explicit way and we already have support for it, another approach is use recommended values from core which basically are actions that can be used in all providers. they are crafted and maintained carefully so without needing any extra efforts to update provider package (e.g phantom), it can receives some recommended actions at anytime easily.
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.
This is my suggestion on how you could improve this approach by having before, after and other hooks composed with action in single object instead of separating them and importing duplicate stuff everywhere.
You could still have separate functions to maintain separation of concerns, but you could export them together as well like this instead of exporting them separately as several recommended methods which is not meaningful as it could produce mistake if a developer forgets to do something and also reduces code readability.
This is not the exact syntax but you could get the idea.
const connect = new ActionBuilder('connect', () => {})
.('after', () => {}) // or .after(() => {})
.('before', () => {})
.('and', () => {})
.build()
const disconnect = new ActionBuilder('disconnect', () => {})
.('after', () => {})
.('before', () => {})
.('blablabla', () => {})
.build()
const evm = new NamespaceBuilder<EvmActions>('evm', WALLET_ID)
.action('init', () => {})
.action([connect, disconnect])
.build();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 created a new builder called ActionBuilder to achieve what you've proposed here. I only applied it to evm namespace for Phantom. If the approach has been finalized, I will update solana namespace for phatom as well.
This comment was marked as resolved.
This comment was marked as resolved.
2338595 to
781ff9a
Compare
…en triggering accountChange
| export function connect( | ||
| instance: () => ProviderApi | ||
| ): ['connect', FunctionWithContext<EvmActions['connect'], Context>] { | ||
| return [ | ||
| 'connect', | ||
| async (_context, chain) => { | ||
| const evmInstance = instance(); |
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.
Can you elaborate how eager connect is implemented in EVM or Solana? I wasn't able to find related code.
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.
Please take a look at wallets/react/src/hub/useAutoConnect.ts for hub, and wallets/react/src/legacy/useAutoConnect.ts for legacy.
…ction in one place
f723f83 to
6453314
Compare
|
When I cancel the connect popup in phantom extension, no error shows up in widget containing the error message. |
RyukTheCoder
left a comment
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 recommend to use provider everywhere instead o fwallet.
| .init(function (context) { | ||
| const [, setState] = context.state(); | ||
|
|
||
| if (phantomInstance()) { |
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.
| if (phantomInstance()) { | |
| if (getPhantomInstance()) { |
| const evmInstance = instances?.get(Networks.ETHEREUM); | ||
|
|
||
| if (!instances || !evmInstance) { | ||
| throw new Error( |
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.
You are not supposed to throw error if the instance is not available. Throwing error should be done on handling an action from user like connecting and signing transactions.
| return instances; | ||
| } | ||
|
|
||
| export function evmPhantom(): EvmProviderApi { |
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.
| export function evmPhantom(): EvmProviderApi { | |
| export function getNetworkInstance(network: Networks) { | |
| const instance = getProviderInstance(); | |
| const networkInstance = instance?.get(network); | |
| if (!networkInstance) { | |
| throw new Error( | |
| `Are you sure ${WALLET_ID} injected and you have enabled ${network} correctly?` | |
| ); | |
| } | |
| return networkInstance; | |
| } |
|
this PR broken into smaller PRs, and they got merged. there is a list including related PRs in first comment |



Introducing Hub
Background
Initially, We developed the first version of our wallet management to meet our internal system demands. Over time, Our library has evolved to support approximately 35 wallets, with more on the horizon.
This period of the development has provided invaluable insights into the nuances of wallet management, revealing accessibility and functionality across different wallet types.
Despite the success of initial implementation, We recognized the need for a more robust solution to address emerging challenges and to accommodate future growth. Our objectives for the new version are:
Moreover, we recognized the importance of making our library beyond our internal systems. So a key objective is to decouple it from a specific infrastructure dependencies (Rango), transforming it to a public library to be used throughout the entire crypto space .
Technical details
All the technical details can be found in
/wallets/core/docs.Known issues
andUsehas a type issue. Details:wallets/core/src/builders/namespace.ts.react-docgenhas been disabled during a bug they have with class private fields.For reviewer
How to review
I suggest the following steps:
migration-guide.mdand starts to migrate a provider. Choose an EVM provider for start.corewalltets-react.Notable Changes
1. Migrating to latest ESM format
We are going to adopt the latest ES Modules standards.
Package entry points
ESM now supports defining multiple entry points for a package. Entry points will be defined in
package.jsonandexportsfield.For example, our
wallets-corehas several entry points:import x from '@rango-dev/wallets-core'import x from '@rango-dev/wallets-core/utils'import x from '@rango-dev/wallets-core/legacy'Read more about the format: https://nodejs.org/api/packages.html#package-entry-points
File extension
File extension is required which means
import x from "./x.js"should be used instead ofimport x from './x'tsconfig changes
These two options should be added into
tsconfig{ "module": "NodeNext", "moduleResolution": "NodeNext" }It also needed to library users to add this option to their
tsconfigif they want to useexportsfrom a lib:{ "moduleResolution": "Bundler" }I tried to migrate
embeddedto useexportsfield as well, but it needs lots of change. I solved it by by supporting both legacy and new format at the same time forwallets-core.Only
wallets-coremigrated toNodeNextfor now, but we can do migrate more packages in future after merging this changes.Changes to build script
our
scripts/buildnow accepts--inputsto accept more than one entry point. This is useful for supportingexports.New package layout
As you can see I intentionally changed the
index.tstomod.ts, the reason behind this:NodeNext.Note: Our libraries will use this format, our clients don't need to follow this conventions.
2. Two
tsconfigfor each PackageWe put our integration in
testsdirectory besidessrc. If we consider the package root as entry point for typescript, it ruins thedistfolder by including bothdist/src/anddist/tests. In summary, we have two phase: distributing package and running code (tests or on local). I couldn't find any straight forward approach to solve this issue.One of workaround for this is having two
tsconfig, When we are going to run tests or developing locally usetsconfig.jsonwhich is default and besides that, usetsconfig.build.jsonwhen we are going to build a package and publish the package. It's been added to/scripts/build.3. Legacy vs Hub (or v1)
What we have on production is called
legacyfrom now on and we call the new implementationhubwhich is going to be our first stable and final version for wallet management.you can see a directory called
/legacyinsideprovider-phantom,wallets-coreandwallets-react. Most of them are moving old implementation into a separate directory with same filename.What to test/check
discoverNamespaceinwallets/react/src/hub/utils.ts, that would be great to make sure i didn't miss anything.Roadmap
core.embeddedto usecoredirectly and remove most of thereactfunctionalities.coreas actions and depreacte those libraries.docusaurusornextrafor public usage.Related PRs