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

UX/ Reduce RPC error banners #1210

Draft
wants to merge 2 commits into
base: v2
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions src/controllers/accounts/accounts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,23 +91,25 @@ export class AccountsController extends EventEmitter {
async updateAccountState(
accountAddr: Account['addr'],
blockTag: 'pending' | 'latest' = 'latest',
networks: NetworkId[] = []
networks: NetworkId[] = [],
isManualUpdate = false
) {
const accountData = this.accounts.find((account) => account.addr === accountAddr)

if (!accountData) return

await this.withStatus(
'updateAccountState',
async () => this.#updateAccountStates([accountData], blockTag, networks),
async () => this.#updateAccountStates([accountData], blockTag, networks, isManualUpdate),
true
)
}

async #updateAccountStates(
accounts: Account[],
blockTag: string | number = 'latest',
updateOnlyNetworksWithIds: NetworkId[] = []
updateOnlyNetworksWithIds: NetworkId[] = [],
isManualUpdate = false
) {
// if any, update the account state only for the passed networks; else - all
const updateOnlyPassedNetworks = updateOnlyNetworksWithIds.length
Expand Down Expand Up @@ -138,6 +140,17 @@ export class AccountsController extends EventEmitter {
})
} catch (err) {
console.error(`account state update error for ${network.name}: `, err)

if (isManualUpdate) {
accounts.forEach((account) => {
const state = this.accountStates?.[account.addr]?.[network.id]

if (!state) return
// Reset the account state updatedAt timestamp so a banner is displayed
state.updatedAt = 0
})
}

this.#updateProviderIsWorking(network.id, false)
}

Expand Down Expand Up @@ -216,4 +229,11 @@ export class AccountsController extends EventEmitter {
await this.#storage.set('accounts', this.accounts)
this.emitUpdate()
}

get areAccountStatesLoading() {
return (
this.statuses.updateAccountStates === 'LOADING' ||
this.statuses.updateAccountState === 'LOADING'
)
}
}
8 changes: 7 additions & 1 deletion src/controllers/main/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,7 @@ export class MainController extends EventEmitter {

this.signAccountOp = new SignAccountOpController(
this.accounts,
this.providers,
this.keystore,
this.portfolio,
this.#externalSignerControllers,
Expand Down Expand Up @@ -963,7 +964,12 @@ export class MainController extends EventEmitter {
// However, even if we don't trigger an update here, it's not a big problem,
// as the account state will be updated anyway, and its update will be very recent.
!isUpdatingAccount && this.selectedAccount.account?.addr
? this.accounts.updateAccountState(this.selectedAccount.account.addr, 'pending')
? this.accounts.updateAccountState(
this.selectedAccount.account.addr,
'pending',
undefined,
true
)
: Promise.resolve(),
// `updateSelectedAccountPortfolio` doesn't rely on `withStatus` validation internally,
// as the PortfolioController already exposes flags that are highly sufficient for the UX.
Expand Down
12 changes: 10 additions & 2 deletions src/controllers/selectedAccount/selectedAccount.ts
Original file line number Diff line number Diff line change
Expand Up @@ -322,12 +322,20 @@ export class SelectedAccountController extends EventEmitter {
}

#updatePortfolioBanners(skipUpdate?: boolean) {
if (!this.account || !this.#networks || !this.#providers || !this.#portfolio) return
if (
!this.account ||
!this.#networks ||
!this.#providers ||
!this.#portfolio ||
this.#accounts.areAccountStatesLoading
)
return

const networksWithFailedRPCBanners = getNetworksWithFailedRPCBanners({
providers: this.#providers.providers,
networks: this.#networks.networks,
networksWithAssets: this.#portfolio.getNetworksWithAssets(this.account.addr)
networksWithAssets: this.#portfolio.getNetworksWithAssets(this.account.addr),
accountState: this.#accounts.accountStates[this.account.addr]
})

const errorBanners = getNetworksWithPortfolioErrorBanners({
Expand Down
1 change: 1 addition & 0 deletions src/controllers/signAccountOp/signAccountOp.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ const init = async (
const callRelayer = relayerCall.bind({ url: '', fetch })
const controller = new SignAccountOpController(
accountsCtrl,
providersCtrl,
keystore,
portfolio,
{},
Expand Down
5 changes: 5 additions & 0 deletions src/controllers/signAccountOp/signAccountOp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import { AccountOpAction } from '../actions/actions'
import EventEmitter from '../eventEmitter/eventEmitter'
import { KeystoreController } from '../keystore/keystore'
import { PortfolioController } from '../portfolio/portfolio'
import { ProvidersController } from '../providers/providers'
import {
getFeeSpeedIdentifier,
getFeeTokenPriceUnavailableWarning,
Expand Down Expand Up @@ -115,6 +116,8 @@ export class SignAccountOpController extends EventEmitter {

#keystore: KeystoreController

#providers: ProvidersController

#portfolio: PortfolioController

#externalSignerControllers: ExternalSignerControllers
Expand Down Expand Up @@ -175,6 +178,7 @@ export class SignAccountOpController extends EventEmitter {

constructor(
accounts: AccountsController,
providers: ProvidersController,
keystore: KeystoreController,
portfolio: PortfolioController,
externalSignerControllers: ExternalSignerControllers,
Expand All @@ -189,6 +193,7 @@ export class SignAccountOpController extends EventEmitter {
super()

this.#accounts = accounts
this.#providers = providers
Copy link
Member

Choose a reason for hiding this comment

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

@PetromirDev maybe you had an idea here and forgot to remove the providers?

this.#keystore = keystore
this.#portfolio = portfolio
this.#externalSignerControllers = externalSignerControllers
Expand Down
1 change: 1 addition & 0 deletions src/interfaces/account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export interface AccountOnchainState {
isErc4337Nonce: boolean
isV2: boolean
currentBlock: bigint
updatedAt: number
}

export type AccountStates = {
Expand Down
3 changes: 2 additions & 1 deletion src/libs/accountState/accountState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ export async function getAccountState(
),
currentBlock: accResult.currentBlock,
deployError:
accounts[index].associatedKeys.length > 0 && accResult.associatedKeyPrivileges.length === 0
accounts[index].associatedKeys.length > 0 && accResult.associatedKeyPrivileges.length === 0,
updatedAt: Date.now()
}

return res
Expand Down
8 changes: 5 additions & 3 deletions src/libs/banners/banners.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Account } from '../../interfaces/account'
import { Account, AccountStates } from '../../interfaces/account'
import { AccountOpAction, Action as ActionFromActionsQueue } from '../../interfaces/actions'
// eslint-disable-next-line import/no-cycle
import { Action, Banner } from '../../interfaces/banner'
Expand Down Expand Up @@ -327,14 +327,16 @@ export const getKeySyncBanner = (addr: string, email: string, keys: string[]) =>
export const getNetworksWithFailedRPCBanners = ({
providers,
networks,
networksWithAssets
networksWithAssets,
accountState
}: {
providers: RPCProviders
networks: Network[]
networksWithAssets: NetworkId[]
accountState: AccountStates[string]
}): Banner[] => {
const banners: Banner[] = []
const networkIds = getNetworksWithFailedRPC({ providers }).filter((networkId) =>
const networkIds = getNetworksWithFailedRPC({ providers, accountState }).filter((networkId) =>
networksWithAssets.includes(networkId)
)

Expand Down
7 changes: 6 additions & 1 deletion src/libs/errorHumanizer/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@ const BROADCAST_OR_ESTIMATION_ERRORS: ErrorHumanizerError[] = [
'the RPC provider does not support the requested operation. Please check your RPC settings or contact the dApp team.'
},
{
reasons: [RPC_HARDCODED_ERRORS.rpcTimeout, 'Unable to connect to provider'],
reasons: [
RPC_HARDCODED_ERRORS.rpcTimeout,
'Unable to connect to provider',
'SERVER_ERROR',
'NETWORK_ERROR'
],
message:
'of a problem with the RPC on this network. Please try again later, change the RPC or contact support for assistance.'
},
Expand Down
25 changes: 20 additions & 5 deletions src/libs/networks/networks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import { AMBIRE_ACCOUNT_FACTORY, OPTIMISTIC_ORACLE, SINGLETON } from '../../consts/deploy'
import { networks as predefinedNetworks } from '../../consts/networks'
import { AccountStates } from '../../interfaces/account'
import { Fetch } from '../../interfaces/fetch'
import {
Erc4337settings,
Expand Down Expand Up @@ -59,11 +60,25 @@ export function is4337Enabled(
return hasBundlerSupport
}

export const getNetworksWithFailedRPC = ({ providers }: { providers: RPCProviders }): string[] => {
return Object.keys(providers).filter(
(networkId) =>
typeof providers[networkId].isWorking === 'boolean' && !providers[networkId].isWorking
)
export const getNetworksWithFailedRPC = ({
providers,
accountState
}: {
providers: RPCProviders
accountState: AccountStates[string]
}): string[] => {
return Object.keys(providers).filter((networkId) => {
const isProviderWorking =
typeof providers[networkId].isWorking !== 'boolean' || providers[networkId].isWorking

if (isProviderWorking) return false
if (!accountState || !accountState[networkId]) return true

const EIGHT_MINUTES = 1000 * 60 * 8
const lastUpdate = accountState[networkId]?.updatedAt || 0

return Date.now() - lastUpdate > EIGHT_MINUTES
})
}

async function retryRequest(init: Function, counter = 0): Promise<any> {
Expand Down
Loading