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

fix: duplicate pop-up authn Iframe #367

Merged
merged 6 commits into from
Jan 11, 2024

Conversation

q20274982
Copy link
Member

@q20274982 q20274982 commented Dec 29, 2023

Summary

  • fix this._blocto updating operation in ethereum provider's #getBloctoProperties()
  • remove isomorphicInitialize() to avoid repeat add eventlistener's callback fn
  • handle action.update({ ... }) in event callback fn to avoid repeat open iframe
  • add method: 'eth_blockNumber & method: 'web3_clientVersion' to non-connected switch blocks to prevent the authn Iframe from opening when uniswap calls eth_blockNumber & web3_clientVersion.

Related Links

Asana:https://app.asana.com/0/1201812548509877/1205974827065486/f

Checklist

  • Pasted Asana link.
  • Tagged labels.

Prerequisite/Related Pull Requests

iShot_2023-12-29_14 51 22

@q20274982 q20274982 added the bug Something isn't working label Dec 29, 2023
@q20274982 q20274982 requested a review from akira02 December 29, 2023 07:06
Copy link

changeset-bot bot commented Dec 29, 2023

🦋 Changeset detected

Latest commit: 0c7bd6c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@blocto/web3-react-connector Patch
@blocto/sdk Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -163,7 +163,8 @@ export default class EthereumProvider
name,
display_name,
network_type,
wallet_web_url: this._blocto.walletServer,
wallet_web_url: this.injectedWalletServer ||
ETH_ENV_WALLET_SERVER_MAPPING[blocto_service_environment],
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 executing this method for the first time, this._blocto.walletServer is an empty string.
use this.injectedWalletServer || ETH_ENV_WALLET_SERVER_MAPPING[blocto_service_environment] to avoid setting an empty string for the first time.

Copy link
Member

@mordochi mordochi Jan 11, 2024

Choose a reason for hiding this comment

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

Maybe extract this.injectedWalletServer || ETH_ENV_WALLET_SERVER_MAPPING[blocto_service_environment] as a constant, and we can just reuse this constant in L155 and L166? This can provide a clearer expression for the relation here, which helps people maintaining the code to understand the code better

@@ -443,6 +444,7 @@ export default class EthereumProvider
case 'wallet_addEthereumChain': {
return this.loadSwitchableNetwork(payload?.params || []);
}
case 'eth_blockNumber':
Copy link
Member Author

Choose a reason for hiding this comment

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

uniswap makes periodic calls to eth_blockNumber, adding eth_blockNumber to avoid falling into the default switch block that requires user to be connected (L527)

@q20274982 q20274982 marked this pull request as ready for review December 29, 2023 07:21
packages/blocto-sdk/src/providers/ethereum.ts Show resolved Hide resolved
adapters/web3-react-connector/src/index.ts Outdated Show resolved Hide resolved
adapters/web3-react-connector/src/index.ts Outdated Show resolved Hide resolved
adapters/web3-react-connector/src/index.ts Outdated Show resolved Hide resolved
@q20274982 q20274982 force-pushed the fix/web3-react-switch-chain-error branch from cd79ae9 to a6cf160 Compare January 8, 2024 05:49
@q20274982 q20274982 requested a review from mordochi January 8, 2024 05:50
@q20274982 q20274982 force-pushed the fix/web3-react-switch-chain-error branch from a6cf160 to 897e8ce Compare January 10, 2024 07:14
mordochi
mordochi previously approved these changes Jan 11, 2024
adapters/web3-react-connector/src/index.ts Show resolved Hide resolved
@@ -163,7 +163,8 @@ export default class EthereumProvider
name,
display_name,
network_type,
wallet_web_url: this._blocto.walletServer,
wallet_web_url: this.injectedWalletServer ||
ETH_ENV_WALLET_SERVER_MAPPING[blocto_service_environment],
Copy link
Member

@mordochi mordochi Jan 11, 2024

Choose a reason for hiding this comment

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

Maybe extract this.injectedWalletServer || ETH_ENV_WALLET_SERVER_MAPPING[blocto_service_environment] as a constant, and we can just reuse this constant in L155 and L166? This can provide a clearer expression for the relation here, which helps people maintaining the code to understand the code better

@q20274982 q20274982 merged commit bf8362c into develop Jan 11, 2024
3 checks passed
@q20274982 q20274982 deleted the fix/web3-react-switch-chain-error branch January 11, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working BugFix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants