Use React context for SolanaConnection #284
Conversation
Deploying with
|
| Latest commit: |
c7803cd
|
| Status: | 🚫 Build failed. |
swimdrew
requested review from
ramenuncle,
nicomiicro and
wormat
and removed request for
nicomiicro
| export const useSolanaConnection = (): SolanaConnection => { | ||
| const { env } = useEnvironment(); | ||
| const { chains } = useEnvironment(selectConfig, shallow); | ||
| const [chain] = chains[Protocol.Solana]; | ||
| const { endpoints } = chain; | ||
|
|
||
| const queryClient = useQueryClient(); | ||
| const queryKey = [env, "solanaConnection"]; | ||
|
|
||
| const connection = | ||
| // used as context cache to avoid multiple instances | ||
| queryClient.getQueryData<SolanaConnection>(queryKey) || | ||
| (function createSolanaConnection(): SolanaConnection { | ||
| const solanaConnection = new SolanaConnection(endpoints); | ||
| queryClient.setQueryData(queryKey, solanaConnection); | ||
| return solanaConnection; | ||
| })(); | ||
|
|
||
| return connection; | ||
| }; |
There was a problem hiding this comment.
Keep this file and move the useSolanaConnection hook here to avoid many code changes
export const useSolanaConnection = (): SolanaConnection => {
const context = useContext(SolanaConnectionContext);
if (!context) {
throw new Error("Missing Solana connection context");
}
return context;
};
It will also prevent test failing
There was a problem hiding this comment.
Fixed, thanks for the cleaner PR.
| // used as context cache to avoid multiple instances | ||
| queryClient.getQueryData<SolanaConnection>(queryKey) || | ||
| (function createSolanaConnection(): SolanaConnection { | ||
| const solanaConnection = new SolanaConnection(endpoints); |
There was a problem hiding this comment.
I've added console.log around here but doesn't see the connection recreate per min.
Maybe the root cause is from somewhere else?
There was a problem hiding this comment.
@ramenuncle I think it is related to the react-query refetch on window focus, try switching tabs after 1 min, it should be triggered
There was a problem hiding this comment.
If you have a strong preference against useContext, the change I feel most strongly about is removing the dummy subscription in SolanaConnection.ts.
There was a problem hiding this comment.
@swimdrew How confident are you we don't need that anymore?
There was a problem hiding this comment.
Checkout this code block, the dummy subscription should be defunct: https://github.com/solana-labs/solana-web3.js/blob/7d05857/src/connection.ts#L4634-L4664
There was a problem hiding this comment.
We could iterate through all actions our app does, but I feel very confident that removing the dummy connection is safe.
- I've read the web3.js Connection class and only the following methods use the class's
rpcWsClient:
- onAccountChange
- onProgramAccountChange
- onLogs
- onSlot{Change, Update}
- onSignature{WithOptions}
- onRootChange
Of these methods, we use onAccountChange to establish the dummy connection to keep rpcWsClient alive, thus there is no actual need to keep rpcWsClient alive.
- In our
/apps/ui/node_modules/.../@solana/web3.js/src/connection.ts, therpcWsClienthas the missing code that the serum code was accounting for, and I thus feel confident that the Connection class correctly opens and closes its WS client.
There was a problem hiding this comment.
OK let's do some manual testing and ask testers to try leaving the app open for a while in between interactions, then let's try it (after a positive review obvs).
There was a problem hiding this comment.
Sorry, it turns out our app DOES use onSignature when bridging tokens from Solana to another chain (from the wormhole library) (tested by Kenny). The swap hangs waiting to confirm the bridge, but it does not hang when swapping from Solana to BSC. Likewise, I don't know with certainty why the dummy connection is still necessary, but restoring it made my AVAX swap go through.
My proposal is to do another round of testing with the dummy connection, and keep the Solana context.
There was a problem hiding this comment.
OK do you want to make a follow-up issue to investigate the dummy connection?
There was a problem hiding this comment.
Yeah maybe Thor can give me his bug, and itll be a bit low priority for me.
|
✨ Deployment complete! Take a peek over at https://c9abffb8.ui-storybook.pages.dev |
Co-authored-by: Chiu <[email protected]>
Use React context instead for SolanaConnection. The current ReactQuery implementation recreates SolanaConnection every minute, resulting in spammy WS usage.
Also remove dummy websocket connection. It is unnecessary because
Notion ticket: Slack thread
Checklist