-
Notifications
You must be signed in to change notification settings - Fork 2
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
Use React context for SolanaConnection #284
Merged
Merged
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
979b097
Merge branch 'master' of github.com:swim-io/swim
swimdrew 7cc0102
Merge branch 'master' of github.com:swim-io/swim
swimdrew 9353c22
feat: use React context for SolanaConnection
swimdrew e6fa53b
fix: less naughty error msg
swimdrew 1e24592
fix: update import
swimdrew bdc38c5
fix: fix some imports and remove dummy websocket connection
swimdrew 72ae60e
fix: import
swimdrew a4cc973
Merge branch 'master' of github.com:swim-io/swim into ui/solanaContext
swimdrew 84d8178
style: restore useSolanaConnection hook and original imports
swimdrew 2474962
fix: revert TestPage to master
swimdrew a4ffa49
Merge branch 'master' of github.com:swim-io/swim into ui/solanaContext
swimdrew ce198a9
Merge branch 'master' of github.com:swim-io/swim into ui/solanaContext
swimdrew fd81699
fix: remove outdated file
swimdrew 8e43757
fix: restore dummy WS subscription
swimdrew 4579ea2
style: fix lint for imports
swimdrew 48dcd84
fix: lint import
swimdrew b49e969
fix: import lint error
swimdrew 43aec07
Merge branch 'master' of github.com:swim-io/swim into ui/solanaContext
swimdrew c7803cd
style: Update apps/ui/src/hooks/solana/useSolanaConnection.ts
swimdrew 6b9ac35
Merge branch 'ui/solanaContext' of github.com:swim-io/swim into ui/so…
swimdrew 3c2455f
fix: reply to PR comments
swimdrew 1ea6027
Merge branch 'master' into ui/solanaContext
wormat File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import type { ReactElement, ReactNode } from "react"; | ||
import { createContext, useMemo } from "react"; | ||
import shallow from "zustand/shallow.js"; | ||
|
||
import { Protocol } from "../config/ecosystem"; | ||
import { selectConfig } from "../core/selectors"; | ||
import { useEnvironment } from "../core/store"; | ||
import { SolanaConnection } from "../models"; | ||
|
||
export const SolanaConnectionContext: React.Context<null | SolanaConnection> = | ||
createContext<null | SolanaConnection>(null); | ||
|
||
export const SolanaConnectionProvider = ({ | ||
children, | ||
}: { | ||
readonly children?: ReactNode; | ||
}): ReactElement => { | ||
const { chains } = useEnvironment(selectConfig, shallow); | ||
const [{ endpoints }] = chains[Protocol.Solana]; | ||
|
||
const connection = useMemo( | ||
() => new SolanaConnection(endpoints), | ||
[endpoints], | ||
); | ||
|
||
return ( | ||
<SolanaConnectionContext.Provider value={connection}> | ||
{children} | ||
</SolanaConnectionContext.Provider> | ||
); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,10 @@ | ||
import type React from "react"; | ||
|
||
import { SolanaConnectionProvider } from "./SolanaConnection"; | ||
import { QueryClientProvider } from "./queryClient"; | ||
|
||
export const AppContext: React.FC = ({ children }) => ( | ||
<QueryClientProvider>{children}</QueryClientProvider> | ||
<SolanaConnectionProvider> | ||
<QueryClientProvider>{children}</QueryClientProvider> | ||
</SolanaConnectionProvider> | ||
); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,12 @@ | ||
import { useQueryClient } from "react-query"; | ||
import shallow from "zustand/shallow.js"; | ||
import { useContext } from "react"; | ||
|
||
import { Protocol } from "../../config"; | ||
import { selectConfig } from "../../core/selectors"; | ||
import { useEnvironment } from "../../core/store"; | ||
import { SolanaConnection } from "../../models"; | ||
import { SolanaConnectionContext } from "../../contexts/SolanaConnection"; | ||
import type { SolanaConnection } from "../../models/solana"; | ||
|
||
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; | ||
const solanaConnection = useContext(SolanaConnectionContext); | ||
if (!solanaConnection) { | ||
throw new Error("Missing Solana connection context"); | ||
} | ||
return solanaConnection; | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swimdrew How confident are you we don't need that anymore?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could iterate through all actions our app does, but I feel very confident that removing the dummy connection is safe.
rpcWsClient
:Of these methods, we use
onAccountChange
to establish the dummy connection to keeprpcWsClient
alive, thus there is no actual need to keeprpcWsClient
alive./apps/ui/node_modules/.../@solana/web3.js/src/connection.ts
, therpcWsClient
has 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK do you want to make a follow-up issue to investigate the dummy connection?
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 maybe Thor can give me his bug, and itll be a bit low priority for me.