Skip to content
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

Feature: Connection switch #105

Closed

Conversation

kevinrodriguez-io
Copy link
Contributor

This feature enables switching the connection.

Screen Shot 2021-09-30 at 01 15 57
Screen Shot 2021-09-30 at 01 16 03

Draft PR. Additional implementations on the way.

Copy link
Collaborator

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I think the implementation can be simplified significantly and the type safety of handling connection endpoints can be improved.

Comment on lines +14 to +18

export function useSetConnection(): (connection: Connection) => void {
const dispatch = useContext(ConnectionDispatchContext);
return (connection: Connection) => dispatch(connection);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems overly complicated. Why not just const [connection, setConnection] = useState(() => new Connection(...)) in the provider and then pass setConnection into the context?

Comment on lines +42 to +51
endpoint: clusterApiUrl(WalletAdapterNetwork.Mainnet),
name: 'Mainnet',
},
{
endpoint: clusterApiUrl(WalletAdapterNetwork.Devnet),
name: 'Devnet',
},
{
endpoint: clusterApiUrl(WalletAdapterNetwork.Testnet),
name: 'Testnet',
Copy link
Collaborator

@jordaaash jordaaash Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names like this should be an enum or some other typesafe structure so that child components are sure to input the correct values. The connections array could be part of the ConnectionProvider since it's not dependent upon UI, and the connection value can be an item in that array.

Comment on lines +6 to +8
connection: {
name: string;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a named interface provided by the context.

Comment on lines +9 to +12
connections: {
name: string;
endpoint: string;
}[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always good to export named interfaces for nested props if they need to be consumed by descendants and downstream devs.

}
) => {
// TODO: Add a way to replace commitment.
setConnection(new Connection(connection.endpoint, { commitment: 'confirmed' }));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this doesn't use the connection options provided by the existing ConnectionProvider context.

import { WalletConnectButton } from './WalletConnectButton';
import { WalletIcon } from './WalletIcon';
import { WalletModalButton } from './WalletModalButton';

export const WalletMultiButton: FC<ButtonProps> = ({ type = 'primary', size = 'large', children, ...props }) => {
const { publicKey, wallet, disconnect } = useWallet();
const { setVisible } = useWalletModal();
const { setVisible: setConnetionModalVisible } = useConnectionModal();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in Connetion

@jordaaash
Copy link
Collaborator

Closing for now since there's been no response. Please comment if you want to reopen!

@jordaaash jordaaash closed this Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
icebox Come back to this later waiting Waiting for response from issue/PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants