diff --git a/src/controllers/main/main.ts b/src/controllers/main/main.ts index 600588431..418def689 100644 --- a/src/controllers/main/main.ts +++ b/src/controllers/main/main.ts @@ -233,7 +233,7 @@ export class MainController extends EventEmitter { async (network: Network) => { this.providers.setProvider(network) await this.accounts.updateAccountStates('latest', [network.id]) - await this.updateSelectedAccountPortfolio(true) + await this.updateSelectedAccountPortfolio() await this.defiPositions.updatePositions(network.id) }, (networkId: NetworkId) => { @@ -354,9 +354,7 @@ export class MainController extends EventEmitter { await this.accounts.initialLoadPromise await this.selectedAccount.initialLoadPromise - // TODO: We agreed to always fetch the latest and pending states. - // To achieve this, we need to refactor how we use forceUpdate to obtain pending state updates. - this.updateSelectedAccountPortfolio(true) + this.updateSelectedAccountPortfolio() this.defiPositions.updatePositions() /** * Listener that gets triggered as a finalization step of adding new @@ -425,9 +423,7 @@ export class MainController extends EventEmitter { } this.selectedAccount.setAccount(accountToSelect) this.activity.init() - // TODO: We agreed to always fetch the latest and pending states. - // To achieve this, we need to refactor how we use forceUpdate to obtain pending state updates. - await this.updateSelectedAccountPortfolio(true) + await this.updateSelectedAccountPortfolio() await this.defiPositions.updatePositions() // forceEmitUpdate to update the getters in the FE state of the ctrl await this.forceEmitUpdate() @@ -925,7 +921,7 @@ export class MainController extends EventEmitter { } // eslint-disable-next-line default-param-last - async updateSelectedAccountPortfolio(forceUpdate: boolean = true, network?: Network) { + async updateSelectedAccountPortfolio(forceUpdate: boolean = false, network?: Network) { await this.#initialLoadPromise if (!this.selectedAccount.account) return @@ -1541,7 +1537,7 @@ export class MainController extends EventEmitter { async addNetwork(network: AddNetworkRequestParams) { await this.networks.addNetwork(network) - await this.updateSelectedAccountPortfolio(true) + await this.updateSelectedAccountPortfolio() } async removeNetwork(id: NetworkId) { diff --git a/src/controllers/portfolio/portfolio.test.ts b/src/controllers/portfolio/portfolio.test.ts index 9cdf0ac72..3711ecb20 100644 --- a/src/controllers/portfolio/portfolio.test.ts +++ b/src/controllers/portfolio/portfolio.test.ts @@ -262,26 +262,20 @@ describe('Portfolio Controller ', () => { }) describe('Latest tokens', () => { - test('Latest tokens are fetched and kept in the controller, while the pending should not be fetched (no AccountOp passed)', (done) => { + test('Latest tokens are fetched and kept in the controller', async () => { const { controller } = prepareTest() - controller.onUpdate(() => { - const latestState = - controller.latest['0xB674F3fd5F43464dB0448a57529eAF37F04cceA5']?.ethereum! - const pendingState = - controller.pending['0xB674F3fd5F43464dB0448a57529eAF37F04cceA5']?.ethereum - if (latestState && latestState.isReady) { - expect(latestState.isReady).toEqual(true) - expect(latestState.result?.tokens.length).toBeGreaterThan(0) - expect(latestState.result?.collections?.length).toBeGreaterThan(0) - expect(latestState.result?.hintsFromExternalAPI).toBeTruthy() - expect(latestState.result?.total.usd).toBeGreaterThan(1000) - expect(pendingState).toBeFalsy() - done() - } - }) + await controller.updateSelectedAccount(account.addr) - controller.updateSelectedAccount(account.addr) + const latestState = controller.latest['0xB674F3fd5F43464dB0448a57529eAF37F04cceA5']?.ethereum! + const pendingState = + controller.pending['0xB674F3fd5F43464dB0448a57529eAF37F04cceA5']?.ethereum! + expect(latestState.isReady).toEqual(true) + expect(latestState.result?.tokens.length).toBeGreaterThan(0) + expect(latestState.result?.collections?.length).toBeGreaterThan(0) + expect(latestState.result?.hintsFromExternalAPI).toBeTruthy() + expect(latestState.result?.total.usd).toBeGreaterThan(1000) + expect(pendingState).toBeDefined() }) // @TODO redo this test diff --git a/src/controllers/portfolio/portfolio.ts b/src/controllers/portfolio/portfolio.ts index be47bbd2c..815d9273f 100644 --- a/src/controllers/portfolio/portfolio.ts +++ b/src/controllers/portfolio/portfolio.ts @@ -537,20 +537,6 @@ export class PortfolioController extends EventEmitter { currentAccountOps && simulatedAccountOps ? !isAccountOpsIntentEqual(currentAccountOps, simulatedAccountOps) : currentAccountOps !== simulatedAccountOps - - // TODO: The `updateSelectedAccount` function is currently always invoked with `forceUpdate: true` from the application code. - // TODO: We decided to implement this improvement once we first address https://github.com/AmbireTech/ambire-app/issues/2335#issue-2345433944 - // We should rewrite this, and state updates should work in the following manner: - // 1. On the Dashboard screen, there is an interval that updates both the latest and pending states. - // Why do we update both states? Even without AccountOps to simulate, fetching the pending state - // is beneficial because if someone sends you a token, you will see it on the Dashboard as a pending token balance. - // 2. On the Dashboard, if the user manually triggers a Portfolio update, we should pass `forceUpdate: true` and update both states, - // regardless of any internal portfolio controller limits or caching. - // 3. When signing a transaction and we have AccountOps, we have the following two scenarios: - // 3.1. The Dashboard interval should continue updating both the latest and pending states, - // and also simulate AccountOps (without further optimizations, such as checking `areAccountOpsChanged` and updating the state only if necessary). - // 3.2. The SignAccountOp screen also has a simulation interval that updates both states and simulates the AccountOps. - // Here, we should apply the `areAccountOpsChanged` optimization and update both states only if the AccountOps have changed or have not been simulated yet. const forceUpdate = opts?.forceUpdate || areAccountOpsChanged const fallbackHints = (this.#previousHints?.fromExternalAPI && @@ -582,29 +568,25 @@ export class PortfolioController extends EventEmitter { }, forceUpdate ), - // Pending state update - // We are updating the pending state, only if AccountOps are changed or the application logic requests a force update - forceUpdate - ? this.updatePortfolioState( - accountId, - pendingState, - network, - portfolioLib, - { - blockTag: 'pending', - previousHints: fallbackHints, - ...(currentAccountOps && { - simulation: { - account: selectedAccount, - accountOps: currentAccountOps - } - }), - isEOA: !isSmartAccount(selectedAccount), - additionalHints - }, - forceUpdate - ) - : Promise.resolve(false) + this.updatePortfolioState( + accountId, + pendingState, + network, + portfolioLib, + { + blockTag: 'pending', + previousHints: fallbackHints, + ...(currentAccountOps && { + simulation: { + account: selectedAccount, + accountOps: currentAccountOps + } + }), + isEOA: !isSmartAccount(selectedAccount), + additionalHints + }, + forceUpdate + ) ]) // Persist latest state in previousHints in the disk storage for further requests