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

Remove direct usage of web3Provider #6200

Merged
merged 17 commits into from
Oct 22, 2024
Merged

Remove direct usage of web3Provider #6200

merged 17 commits into from
Oct 22, 2024

Conversation

jinchung
Copy link
Member

@jinchung jinchung commented Oct 13, 2024

Fixes APP-1925

What changed (plus any additional context for devs)

  • As part of removing the direct import and usage of web3Provider, I made some functions require a mandatory provider param
  • Moved over to using the getProvider function

Screen recordings / screenshots

Sending:
https://github.com/user-attachments/assets/9448ddff-7413-43c2-847b-3075125599e7

Offline toast:
https://github.com/user-attachments/assets/b361229c-65f8-434f-b96e-c795cf3cab76

What to test

  • sending on various networks
  • executing swap transactions on various networks
  • WalletConnect transactions and message signing
  • speed up and cancel txns across various networks
  • check OfflineToast behaves as expected
  • updating Showcase behaves as expected

Copy link

linear bot commented Oct 13, 2024


const OfflineToast = () => {
const isConnected = useInternetStatus();
const { network } = useAccountSettings();
const providerUrl = web3Provider?.connection?.url;
const isMainnet = network === Network.mainnet && !providerUrl?.startsWith('http://');
Copy link
Member Author

Choose a reason for hiding this comment

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

the only reason the web3Provider was used here was to check the provider URL to see if it's the hardhat URL; moved it over to using the new connectedToHardhat instead

@@ -140,7 +140,7 @@ export const PointsProfileProvider = ({ children }: { children: React.ReactNode
Alert.alert(i18n.t(i18n.l.points.console.generic_alert));
throw new RainbowError('Points: Error loading wallet');
}
const signatureResponse = await signPersonalMessage(challenge, wallet, provider);
const signatureResponse = await signPersonalMessage(challenge, provider, wallet);
Copy link
Member Author

Choose a reason for hiding this comment

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

had to switch the order of the arguments here as signPersonalMessage no longer has an optional provider but still can have an optional wallet arg

@@ -507,15 +498,15 @@ export const SignTransactionSheet = () => {
fn: signPersonalMessage,
screen: SCREEN_FOR_REQUEST_SOURCE[source],
operation: TimeToSignOperation.SignTransaction,
})(message, existingWallet);
})(message, provider, existingWallet);
Copy link
Member Author

Choose a reason for hiding this comment

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

had to switch the order of the arguments for both signPersonalMessage and signTypedDataMessage as they no longer have an optional provider but still can have an optional wallet arg

const initProvider = async () => {
let p;
if (chainId === ChainId.mainnet) {
p = await getFlashbotsProvider();
Copy link
Member Author

Choose a reason for hiding this comment

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

this hook was only used in the SignTransactionSheet for WalletConnect and should not be needing to use the flashbots provider for this flow; moved the fetchNativeAsset logic (lines 27) into SignTransactionSheet itself

@@ -530,7 +530,8 @@ const listenOnNewMessages =

return;
} else if (!isSigningMethod(payload.method)) {
sendRpcCall(payload)
const provider = getProvider({ chainId: ChainId.mainnet });
Copy link
Member Author

@jinchung jinchung Oct 15, 2024

Choose a reason for hiding this comment

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

always using ChainId.mainnet is technically not correct but we will be removing this file shortly and it is a no-op to what it was using previously

* @return The corresponding `TransactionResponse`, or `null` if one could not
* be found.
*/
export const getTransaction = async (hash: string): Promise<TransactionResponse | null> => web3Provider?.getTransaction(hash) ?? null;
Copy link
Member Author

@jinchung jinchung Oct 15, 2024

Choose a reason for hiding this comment

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

these 2 functions (getTransaction and getTransactionCount`) were just unused functions

* @return The response from the `StaticJsonRpcProvider.send` call.
*/
export const sendRpcCall = async (
payload: {
method: string;
params: unknown[];
},
provider: StaticJsonRpcProvider | null = null
Copy link
Member Author

Choose a reason for hiding this comment

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

in this file, most of the changes are related to removing the optional nature for provider in the args - now it is required to provide a provider so take a look at the different places that use these functions to make sure they pass in a valid provider

@@ -268,7 +261,7 @@ export const loadWallet = async <S extends Screen>({
}: {
address?: EthereumAddress;
showErrorIfNotLoaded?: boolean;
provider?: Provider;
provider: Provider;
Copy link
Member Author

@jinchung jinchung Oct 15, 2024

Choose a reason for hiding this comment

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

a lot of changes in this file are also related to making provider arg non-optional; please pay attention to where these functions are being called and that they're passing in non-null provider objects

@jinchung jinchung marked this pull request as ready for review October 16, 2024 18:18
Comment on lines -255 to -257
const currentChainId = await getChainId();
web3SetHttpProvider(currentChainId);
saveChainId(currentChainId);
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

when we first introduced this remote config logic, we needed to keep the legacy web3Provider object in sync - now we are just relying on the backend networks endpoint for the provider info

Copy link
Member

@benisgold benisgold left a comment

Choose a reason for hiding this comment

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

just requesting changes bc of my first comment.

as an aside, i think it's unnecessary to pass in provider as a param in many places. why do components that are not directly using the provider need to have any knowledge of it? it would make more sense/be more readable to just pass in chainId (if needed, often times this can be extracted from other params such as tx payload), and call getProvider at the lowest level possible.

src/components/toasts/OfflineToast.js Outdated Show resolved Hide resolved
src/screens/SignTransactionSheet.tsx Outdated Show resolved Hide resolved
@brunobar79
Copy link
Member

Launch in simulator or device for f3334eb

Copy link
Contributor

@greg-schrammel greg-schrammel left a comment

Choose a reason for hiding this comment

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

approved since what I noticed ben already commented

@jinchung jinchung requested a review from benisgold October 17, 2024 16:06
@brunobar79
Copy link
Member

Launch in simulator or device for afec91f

Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

All looks well with the exception of wallet connect and dapp browser. I can sign txn's but I cannot send. When I tap the confirm button, nothing happens. Cancel does work.

iOS tested on = 1.9.42 (54)
Android tested on release.apk

This branch:
https://github.com/user-attachments/assets/3ccc8f19-56e1-4695-a782-7ae22fc9706b

Compared to prod:
https://github.com/user-attachments/assets/0b163337-ddb0-4d29-96d9-dd7273ed2e9b

Steps to reproduce:

  • navigate to our dapp via Wallet connect and our in-app dapp browser = https://rainbowkit-example.vercel.app/
  • Connect your wallet
  • Once connected to wallet & network, tap send transaction
  • attempt to confirm

Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Fix looks good, QA Passed 👍🏽

@jinchung jinchung merged commit 95df799 into develop Oct 22, 2024
7 of 8 checks passed
@jinchung jinchung deleted the @jin/web3provider branch October 22, 2024 12:36
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.

5 participants