[DO NOT MERGE]: testing bugbot changes #7785
Conversation
![cursor[bot]](https://d.iamgay.cn/https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| describe('addCustomAsset', () => { | ||
| it('adds a custom asset to an account', async () => { | ||
| // eslint-disable-next-line jest/no-focused-tests | ||
| it.only('adds a custom asset to an account', async () => { |
There was a problem hiding this comment.
Focused test will skip all other tests
High Severity
The it.only() modifier on the test case will cause Jest to skip all other tests in this file. This is typically used during local development and debugging but was accidentally committed. The eslint-disable comment jest/no-focused-tests confirms this was intentionally bypassed, which is dangerous in a PR.
| } | ||
| }); | ||
|
|
||
| this.state.assetsDetails[normalizedAssetId] = 'custom'; |
There was a problem hiding this comment.
Direct state mutation outside update callback
High Severity
The line this.state.assetsDetails[normalizedAssetId] = 'custom' directly mutates state outside the update() callback. In BaseController, state modifications must go through update() to trigger state change events and maintain immutability. This assignment occurs after the update() call completes, bypassing proper state management.
| emit(event: string): void { | ||
| this.#listeners.get(event)?.forEach((fn) => fn()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Controller uses EventEmitter instead of messenger pattern
Low Severity · Bugbot Rules
Please review the controller guidelines and follow the recommendations. The SimpleEventEmitter class and hub property violate the guideline "Use the messenger instead of event emitters" (section in controller-guidelines.md). The dataRefreshed event should be published via the messenger instead.
Additional Locations (1)
| @@ -1,3 +1,5 @@ | |||
| export * from './logger'; | |||
There was a problem hiding this comment.
Barrel export violates coding guidelines
Low Severity · Bugbot Rules
Please review the controller guidelines and follow the recommendations. The line export * from './logger' is a barrel export which violates the guideline "Avoid barrel exports...Instead, explicitly name each export."
| await withController(async ({ controller }) => { | ||
| controller.getActiveAssetIds(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test missing assertions and misplaced in wrong describe block
Low Severity · Bugbot Rules
Please review the unit testing guidelines and follow the recommendations. The test 'getActiveAssetIds returns keys from assetsMetadata' has no assertions (suppressed via jest/expect-expect) and is incorrectly nested under the describe('setAssetPrice') block despite testing getActiveAssetIds. Tests without assertions provide no value.
| state.assetsPrice[assetId] = price; | ||
| }); | ||
| this.hub.emit('dataRefreshed'); | ||
| } |
There was a problem hiding this comment.
Missing asset ID normalization causes lookup inconsistency
Medium Severity
The setAssetPrice method stores prices using the raw assetId parameter without calling normalizeAssetId(). Other methods like addCustomAsset, removeCustomAsset, and normalizeResponse all normalize asset IDs to ensure checksummed EVM addresses. If a price is stored with a lowercase address like eip155:1/erc20:0xa0b86991..., but metadata uses the checksummed form 0xA0b86991..., the price lookup in #getAssetsFromState will fail to match.
| emit(event: string): void { | ||
| this.#listeners.get(event)?.forEach((fn) => fn()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused SimpleEventEmitter class and hub property
Medium Severity
The SimpleEventEmitter class is defined and instantiated as hub, and hub.emit('dataRefreshed') is called in setAssetPrice, but there are no subscribers anywhere in the codebase (hub.on(...) is never called for this controller). This creates dead code that adds unnecessary complexity. If event emission is needed, either add the corresponding subscriptions or remove this infrastructure entirely.
Additional Locations (1)
| customAssets: { [accountId: string]: string[] }; | ||
| assetsDetails: { [assetId: string]: string }; | ||
| assetCount: number; | ||
| internalCache: { [key: string]: Json }; |
There was a problem hiding this comment.
Unused state properties assetCount and internalCache
Low Severity
The state properties assetCount and internalCache are defined in the state type, initialized in getDefaultAssetsControllerState, and have metadata configured, but they are never read from or written to anywhere in the codebase. This is dead code that clutters the state structure. Similarly, assetsDetails is only written to (via a buggy direct mutation) but never read.
Explanation
A few changes made by AI that don't follow the standards of the repo to test the bugbot behaviors.
References
Checklist
Note
Medium Risk
Adds new state fields and new APIs/events in
AssetsController, which may affect consumers that assume the previous state shape or rely on messenger-based events. Test changes also introduce a focusedit.only, which could inadvertently skip the rest of the suite if merged.Overview
AssetsController now exposes additional state and update hooks. The controller state gains
assetsDetails,assetCount, andinternalCache(with updated default state and metadata), and the constructor optionally acceptsonStateChangeto receiveAssetsController:stateChangeupdates.Adds a manual price update API and a local refresh signal. New
setAssetPricemutatesassetsPriceand emits adataRefreshedsignal via a new internalhub(SimpleEventEmitter), plusgetActiveAssetIdsto list assets fromassetsMetadata. The PR also tweaks typing in middleware execution (state as unknown as AssetsControllerStateInternal) and marks newly added custom assets inassetsDetails.Tests and tooling: updates
AssetsControllertests for the new state shape and adds coverage forsetAssetPrice/getActiveAssetIds, and introduces.cursor/BUGBOT.mdPR review guidelines.Written by Cursor Bugbot for commit 82dc3bf. This will update automatically on new commits. Configure here.