feat: expose useWallet hook from the public folder #25

sviderock · 2025-01-28T13:21:45Z

Description

This is a proposal for the initial structure of the exposed data hooks that provides better UX for builders by abstracting selectors. Its intention is to hide the complexity of our redux state and provide as much useful info for builders as possible. This will also give us an ability to optimize existing selectors (e.g. by memoizing deps) without re-writing them and without pushing that memoization management on the builders.
Currently, this only exports useWallet to gather some feedback on whether this approach is viable and if we should proceed with it.

There were some hurdles with the module resolution as useWallet is the first file from src/public folder that uses absolute internal paths like src/* for its imports. Including this in the build of @interaxyz/mobile module involves including a lot of internal files that have to be properly resolved.

Reasoning for most of the code is explained in the comments below. Please, feel free to share your feedback!

Currently suggested proposal of exposed data hooks looks like this:

function Component(props: Props) {
  const { address, tokens } = useWallet()
  
  return (
    <View>
      <Text>Wallet address: {address}</Text>
      
      {tokens.map(tokenInfo => (
        <Text>{tokenInfo.symbol}</Text>
      ))}
    </View>
  )
}

Test plan

N/A

Related issues

Relates to RET-1280

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

sviderock · 2025-01-28T13:28:33Z

packages/@interaxyz/mobile/src/public/hooks/useWallet.ts

+import { type NetworkId } from 'src/transactions/types';
+import { walletAddressSelector } from 'src/web3/selectors';
+
+function useTokens() {


This is not moved to a separate file as it's internal use only. This one should only be used in useWallet and the idea for the whole src/public/hooks folder to only contain files with hooks that are gonna be exported.

sviderock · 2025-01-28T13:36:05Z

packages/@interaxyz/mobile/src/public/hooks/useWallet.ts

+  return tokens
+}
+
+export function useWallet() {


The idea is to have clearly defined named hooks that will include grouped information based on the context of the data. E.g. the idea for useWallet is to have:

wallet address

account related to this address (removed for now after discussion with @jeanregisser but possibly gonna revisit)

tokens that are associated with this wallet

nfts? (tbd)

dapp positions? (tbd)

other data? (tbd)

This might introduce a bit cumbersome internal code as it will involve a lot of various selectors but I have a strong feeling that this will provide a lot easier UX for builders.

The idea is to have clearly defined named hooks

i wonder if in practice, we'll either be tempted to throw everything in useWallet or sink a bunch of time into debating about naming a new hook 😅 i kind of prefer having one hook that exposes redux information, assuming that we won't need to expose that much?

@kathaypacific hmm, I feel like that's a double-edged sword of a kind.
One one hand - yes, there's definitely gonna be a lot of stuff that we would often have a thought like "hmm, that would be very convenient to be here; oh, that would fit perfectly in this hook" cause those hooks definitely have a potential to grow out of proportions and become very bloated. That will definitely require more thoughtfulness and consideration from us whenever we add stuff to those hooks.

But IMHO it has a very big benefit in simplicity for the builders that are not aware of our state structure. E.g. we have ~30 selectors for tokens. Obviously, we're not gonna expose all of them. One of them that we would need to expose is sortedTokensWithBalanceOrShowZeroBalanceSelector which has pretty explicit name but it doesn't provide enough context:

Are these tokens that the wallet contains?

Or are these tokens that we support?

Why do we need to pass supported networks as an argument? Why can't those arguments be taken automatically if we configure them in appConfig?

Would every selector need to have these arguments explicitly passed?

Answering most of these questions will probably involve some kind of abstraction of some selectors, be it simple renaming of a selector for clarity or omitting some arguments as they will be taken from appConfig. But those will still involve at least some level of abstraction for which, I believe, we need to find the optimal solution for us.

So having a single hook that exports everything does the same thing useWallet does but in a less granular way, affects tree-shaking capabilities and will still require some kind of abstraction anyways.

I like trying to come up with an interface that will be easy to use and understand for end builders.
Because it's hard to anticipate the different use cases at this stage, I think we could agree to not debate too much about this right now and revisit in a couple of weeks as we refine things before publishing a stable release.

sviderock · 2025-01-28T13:42:02Z

packages/@interaxyz/mobile/src/account/reducer.ts

@@ -1,17 +1,17 @@
 import { Actions, ActionTypes } from 'src/account/actions'
 import { Actions as AppActions, ActionTypes as AppActionTypes } from 'src/app/actions'
+import { DEV_SETTINGS_ACTIVE_INITIALLY } from 'src/config'


This file and its changes is redundant for this PR. I was working on it initially while adding full account data to useWallet and autosave just optimized the imports and not I cannot save the file without it auto-optimizing the imports 🤷

Could you leave it out of the committed changes?
Just to avoid conflicts.

sviderock · 2025-01-28T14:28:19Z

apps/example/package.json

@@ -101,7 +101,7 @@
    "ethers": "^5.7.2",
    "expect": "^29.7.0",
    "lodash": "^4.17.21",
-    "ts-node": "^10.9.2",
+    "ts-node": "11.0.0-beta.1",


As per this issue current stable version of ts-node doesn't support multiple configs in tsconfig extends prop even though it is supported since TS 5.0.

There is a suggestion there that upgrading it to version 11 beta supports multiple version so I've tried it and all the github workflows and scripts from package.json that use ts-node seems to be working and all checks pass successfully. I think it is safe to upgrade?

sviderock · 2025-01-28T14:38:49Z

apps/example/tsconfig.json

+  "extends": [
+    "../../node_modules/expo/tsconfig.base.json",
+    "../../node_modules/@interaxyz/mobile/tsconfig.json"
+  ],


ts-node v10 doesn't support array for extends as per the comment above

The way array value for extends works ints-node v11 is not identical to the way TS interprets the field:

if we wouldn't have used ts-node we'd be able to define it like this:

"extends": [ "expo/tsconfig.base", // this is fine cause TS can resolve the path properly "../../node_modules/@interaxyz/mobile/tsconfig.json" ],

but ts-node implementation of multiple extends cannot resolve absolute paths so it needs to be relative

Using ../../node_modules/@interaxyz/mobile/tsconfig.json here ensures that all the internal imports like src/* from within @interaxyz/mobile are properly resolved. Otherwise, lots of errors would be thrown from useWallet as this is the first file from @interaxyz/mobile/src/public that uses absolute internal paths like src/*.

Using ../../node_modules/@interaxyz/mobile/tsconfig.json also ensures that we don't need to maintain multiple files that overwrite paths resolving (e.g. babel.config.js)

Could you leave a comment that we'll remove the need for this "soon"?
and that we eventually want to only have "extends": "expo/tsconfig.base", in it.
Similar to https://github.com/interaxyz/intera-mobile/blob/715bfab58fccc1ad55a62ea4fe366d01ba549e01/apps/example/babel.config.js#L13

I think ts-node is only needed for the e2e scripts here.

Would it work to add another tsconfig.json in the e2e folder to avoid the specific issues with ts-node and an array of extends?

Then could we do this?

"extends": [ "expo/tsconfig.base", "@interaxyz/mobile/tsconfig.json" ],

I'll check it out!

sviderock · 2025-01-28T14:40:47Z

apps/example/tsconfig.json

+    "jsx": "react-native"
+  },
+  "exclude": ["node_modules", "babel.config.js", "metro.config.js", "jest.config.js"]


These are copied from expo/tsconfig.base as using multiple values in extends gives priority for the last config in the array. This means that these configs will be overwritten by the same configs from "../../node_modules/@interaxyz/mobile/tsconfig.json"

sviderock · 2025-01-28T14:41:19Z

knip.ts

@@ -33,6 +33,7 @@ const config: KnipConfig = {
        'babel-plugin-module-resolver', // not listed in package.json so we use the version from the runtime for now
        'ts-node', // used in workflows run by github actions from the example app dir
        '@walletconnect/core', // used in e2e tests via @walletconnect/sign-client
+        'tslib', // for some reason this is triggered after applying multiple tsconfigs to "extends" of apps/example/tsconfig.json


I'm not sure why this happens but this started to appear once I added multiple configs to extends.

jeanregisser

Awesome! 🚀

left a couple of small things, but already approved to get things moving.

jeanregisser · 2025-01-29T10:58:15Z

apps/example/tsconfig.json

+  "extends": [
+    "../../node_modules/expo/tsconfig.base.json",
+    "../../node_modules/@interaxyz/mobile/tsconfig.json"
+  ],


Could you leave a comment that we'll remove the need for this "soon"?
and that we eventually want to only have "extends": "expo/tsconfig.base", in it.
Similar to https://github.com/interaxyz/intera-mobile/blob/715bfab58fccc1ad55a62ea4fe366d01ba549e01/apps/example/babel.config.js#L13

jeanregisser · 2025-01-29T11:03:02Z

apps/example/tsconfig.json

+  "extends": [
+    "../../node_modules/expo/tsconfig.base.json",
+    "../../node_modules/@interaxyz/mobile/tsconfig.json"
+  ],


I think ts-node is only needed for the e2e scripts here.

Would it work to add another tsconfig.json in the e2e folder to avoid the specific issues with ts-node and an array of extends?

Then could we do this?

"extends": [ "expo/tsconfig.base", "@interaxyz/mobile/tsconfig.json" ],

jeanregisser · 2025-01-29T11:03:57Z

packages/@interaxyz/mobile/src/account/reducer.ts

@@ -1,17 +1,17 @@
 import { Actions, ActionTypes } from 'src/account/actions'
 import { Actions as AppActions, ActionTypes as AppActionTypes } from 'src/app/actions'
+import { DEV_SETTINGS_ACTIVE_INITIALLY } from 'src/config'


Could you leave it out of the committed changes?
Just to avoid conflicts.

jeanregisser · 2025-01-29T11:08:08Z

packages/@interaxyz/mobile/src/public/hooks/useWallet.ts

+import { useSelector } from 'src/redux/hooks'
+import { sortedTokensWithBalanceOrShowZeroBalanceSelector } from 'src/tokens/selectors'
+import { getSupportedNetworkIdsForSend } from 'src/tokens/utils'
+import { type NetworkId } from 'src/transactions/types'
+import { walletAddressSelector } from 'src/web3/selectors'


@sviderock and I chatted about not doing direct imports because of the way the runtime currently needs to be initialized.
I'll document this for the team so we can discuss further and decide on the best long term approach.

jeanregisser · 2025-01-29T12:20:34Z

packages/@interaxyz/mobile/src/public/hooks/useWallet.ts

+  return tokens
+}
+
+export function useWallet() {


I like trying to come up with an interface that will be easy to use and understand for end builders.
Because it's hard to anticipate the different use cases at this stage, I think we could agree to not debate too much about this right now and revisit in a couple of weeks as we refine things before publishing a stable release.

sviderock · 2025-01-29T13:09:21Z

packages/@interaxyz/mobile/src/public/hooks/useWallet.ts

+/**
+ * With the current structure of the @interaxyz/mobile package there is a requirement to initialize
+ * missingGlobals.ts before everything else. It is imported in the src/public/createApp.ts.
+ * For this reason we cannot use top level imports using absolute internal paths like `src/redux/hooks`
+ * as it can cause those resolved paths to be included in the runtime before the globals file.
+ * The same issue happens if we try to use absolute imports like '@interaxyz/mobile/src/redux/hooks'.
+ *
+ * Until we resolve this issue, the approach to include any code from src folder should be as following:
+ *   - top level imports of external modules are allowed (e.g. 'react')
+ *   - internal modules should be included with an inline require
+ *   - "import type" can be used to persist type-safety as it doesn't include types in the runtime
+ *   - when using "import type" - use relative paths (e.g. ../../redux/hooks)
+ */


@jeanregisser I've added some explanatory comment here, please let me know if it is sufficient enough until you're doc is ready!

Perfect thanks!

### Description Exposing earnPositions following the pattern proposed in the initial useWallet PR #25 (comment) ### Test plan Copy/Pasted into node_modules of personal app and verified that it works.

sviderock added 6 commits January 27, 2025 16:32

initial commit

5ca224b

fix tsconfigs

0cc6f0c

Merge branch 'main' into slava/expose-wallet-related-hooks

33ed0d5

lint

19701c4

remove full account selector for now

2253fad

fix new line

214f1c8

sviderock commented Jan 28, 2025

View reviewed changes

sviderock added 2 commits January 28, 2025 15:39

run linter

164e3ac

Merge branch 'main' into slava/expose-wallet-related-hooks

35d89b0

sviderock commented Jan 28, 2025

View reviewed changes

sviderock added 2 commits January 28, 2025 15:59

try to update to ts-node 11 beta

530e5f4

temporary fix knip

047ee50

sviderock commented Jan 28, 2025

View reviewed changes

sviderock added 2 commits January 29, 2025 13:52

Merge branch 'main' into slava/expose-wallet-related-hooks

5516a34

change runtime imports to inline require

1c35212

jeanregisser approved these changes Jan 29, 2025

View reviewed changes

sviderock added 4 commits January 29, 2025 14:28

add explanation

2a6cffc

revert unnecessary change

781217a

fix again

cae55d5

move e2e-related tsconfig to a separate file

656915d

sviderock commented Jan 29, 2025

View reviewed changes

sviderock added this pull request to the merge queue Jan 29, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 29, 2025

sviderock added this pull request to the merge queue Jan 29, 2025

Merged via the queue into main with commit 86f747d Jan 29, 2025
11 checks passed

sviderock deleted the slava/expose-wallet-related-hooks branch January 29, 2025 14:46

jh2oman mentioned this pull request Feb 11, 2025

feat(hooks): expose earnPositions on the useWallet hook #76

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose useWallet hook from the public folder #25

feat: expose useWallet hook from the public folder #25

sviderock commented Jan 28, 2025 •

edited

Loading

sviderock Jan 28, 2025

sviderock Jan 28, 2025 •

edited

Loading

kathaypacific Jan 29, 2025

sviderock Jan 29, 2025 •

edited

Loading

jeanregisser Jan 29, 2025

sviderock Jan 28, 2025 •

edited

Loading

jeanregisser Jan 29, 2025

sviderock Jan 28, 2025 •

edited

Loading

sviderock Jan 28, 2025 •

edited

Loading

jeanregisser Jan 29, 2025

jeanregisser Jan 29, 2025

sviderock Jan 29, 2025

sviderock Jan 28, 2025

sviderock Jan 28, 2025 •

edited

Loading

jeanregisser left a comment

jeanregisser Jan 29, 2025

jeanregisser Jan 29, 2025

jeanregisser Jan 29, 2025

jeanregisser Jan 29, 2025

jeanregisser Jan 29, 2025

sviderock Jan 29, 2025

jeanregisser Jan 29, 2025

feat: expose useWallet hook from the public folder #25

feat: expose useWallet hook from the public folder #25

Conversation

sviderock commented Jan 28, 2025 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

Choose a reason for hiding this comment

sviderock Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

sviderock Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock commented Jan 28, 2025 •

edited

Loading

sviderock Jan 28, 2025 •

edited

Loading

sviderock Jan 29, 2025 •

edited

Loading

sviderock Jan 28, 2025 •

edited

Loading

sviderock Jan 28, 2025 •

edited

Loading

sviderock Jan 28, 2025 •

edited

Loading

sviderock Jan 28, 2025 •

edited

Loading