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

wallet connection #70

Merged
merged 4 commits into from
Jan 24, 2024
Merged

wallet connection #70

merged 4 commits into from
Jan 24, 2024

Conversation

KillariDev
Copy link

@KillariDev KillariDev commented Jan 18, 2024

  • If wallet connection is available always use it. Except for mainnet connection to retrieve ENS names, then we are using hard coded RPC's
  • If keydonix server fails or performs poorly, use other rpc server

@KillariDev KillariDev marked this pull request as ready for review January 19, 2024 12:52
@KillariDev KillariDev requested a review from MicahZoltu January 19, 2024 12:52
Copy link

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

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

This is a pretty significant change. How confident are you that it is the smallest change possible to achieve the desired goals? In particular, the routing/quoting code changes make me nervous.

return useCallback(
async (listUrl: string, skipValidation?: boolean) => {
const requestId = nanoid()
dispatch(fetchTokenList.pending({ requestId, url: listUrl }))
return getTokenList(
listUrl,
(ensName: string) => resolveENSContentHash(ensName, providers[ChainId.MAINNET]),
(ensName: string) => resolveENSContentHash(ensName, RPC_PROVIDERS[ChainId.MAINNET]),

Choose a reason for hiding this comment

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

Can we have this use built-in only if no wallet or wallet is not mainnet? That way if you are using Uniswap on Ethereum Mainnet it will use your wallet for ENS.

Copy link
Author

Choose a reason for hiding this comment

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

yeah we can, I just felt this PR was big enough not to add more stuff to it

Comment on lines 20 to 23
cachedProviderRouter !== undefined &&
cachedProviderRouter.chainId === providerChainId &&
chainId === providerChainId &&
web3Provider === cachedProviderRouter.routerProvider.provider

Choose a reason for hiding this comment

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

Couldn't you just do the last of these 4 checks only and it would be sufficient?

} else {
cachedProviderRouter = undefined
}
if (providerChainId !== undefined && chainId === providerChainId && web3Provider !== undefined) {

Choose a reason for hiding this comment

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

Last check is redundant. If providerChainId is defined then web3Provider will never be undefined. I'm a bit sad that TypeScript doesn't notice this.

Suggested change
if (providerChainId !== undefined && chainId === providerChainId && web3Provider !== undefined) {
if (providerChainId !== undefined && chainId === providerChainId) {

But maybe you need the check in order for TS to properly narrow?

Copy link
Author

Choose a reason for hiding this comment

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

its needed for typescript

@@ -113,10 +113,9 @@ export default class AppRpcProvider extends AppStaticJsonRpcProvider {
for (const { provider, performance } of this.providerEvaluations) {
performance.callCount++
try {
return await provider.perform(method, params)
return useSend ? await provider.send(method, params as Array<any>) : await provider.perform(method, params)

Choose a reason for hiding this comment

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

Did you figure out what send vs perform means?

Copy link
Author

Choose a reason for hiding this comment

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

send sends the arguments as raw as they are defined by caller.
with perform you can name the variables, eg:
return [ "eth_getStorageAt", [ getLowerCase(params.address), hexZeroPad(params.position, 32), params.blockTag ] ];

I don't like this idea, as its all badly typed (all those params. values are any's), but I didn't want to start to refactor it

Choose a reason for hiding this comment

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

Why did all of this need to change, and in pretty significant ways. Feels like this code has little/nothing to do with RPC selection?

Copy link
Author

Choose a reason for hiding this comment

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

because i need to tear down the API logic, that takes only text input, and I cannot convert the web3provider to text

Copy link
Author

Choose a reason for hiding this comment

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

A lot of this logic was handled in createApi side, but I didn't figure out a way to pass the web3provider to it

@KillariDev
Copy link
Author

This is a pretty significant change. How confident are you that it is the smallest change possible to achieve the desired goals? In particular, the routing/quoting code changes make me nervous.

I am not confident at all that this was the smallest change possible, it wasn't really my design goal either. My main intention was to get the wallet connection working for all the crucial places, so that the hard coded RPC's are not mandatory

@KillariDev KillariDev merged commit 42996e9 into main Jan 24, 2024
14 checks passed
@KillariDev KillariDev deleted the wallet-connection branch January 24, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants