-
-
Notifications
You must be signed in to change notification settings - Fork 255
add tron filtering options to token-selectors #7198
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
add tron filtering options to token-selectors #7198
Conversation
0e70cad to
e6a0939
Compare
| expect(result).toStrictEqual(expectedMockResult); | ||
| }); | ||
|
|
||
| const arrangeTronState = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really hate this test suite...
Like sure this uses realistic state data, but MAN its ridiculously difficult to add new tests since you need to understand the WHOLE STATE.
It would be much much nicer if we can:
- Create JSON objects for each state we want to test. We have a fixtures folder that we can shove this in.
- Break up the selector parts into modular atomic functions that we can mock and test in isolation.
| (state) => state.accountTree, | ||
| ( | ||
| _state, | ||
| opts: SelectAccountGroupAssetOpts = defaultSelectAccountGroupAssetOpts, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for now, but I'm not a big fan of this as it can force recalculations based on filters. Also, if we add more chains that have different needs we are going to end up with very messy logic here.
IMO, the problem with Tron should have been solved by expanding the asset type to contain underlying metada (the tron resources), so all these Tron resources would have been added inside the main Tron asset, which will always be present. By adding those "resources" to the list of assets, even though we don't want to display them most of the time, we are causing situations like this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I 100% agree with this.
R.E. selector recalculations - yep noticed this too. Converted to a weakmap memo on arguments to reduce this. Likewise I decoupled the filtering logic, but I can imagine this selector getting real messy down the line.
TRON tokens are a bit of a mess, I agree it would be good if either:
- There was a separation between normal tokens and staking tokens
- Tron team does not expose staking tokens immediately (most flows should not show these tokens), only on some rare screens should they be visible (which can be a separate call not going through our controllers)
Explanation
Currently we have fragmented logic in mobile and extension selectors due to TRON staked assets (energy and bandwidth).
This PR unifies this logic via an optional selector option to filter out TRON staked assets.
Default behaviour is to filter TRON staked assets.
References
Checklist
Note
Adds an optional (default on) filter to exclude Tron staking resources in
selectAssetsBySelectedAccountGroup, with tests and changelog update.TRON_RESOURCE,TRON_RESOURCE_SYMBOLS_SET).selectAssetsBySelectedAccountGroupto accept options{ filterTronStakedTokens }(defaults totrue) and filter Tron staking tokens acrossTrxScopenetworks.weakMapMemoize.src/selectors/__fixtures__/arrange-tron-state.ts.token-selectors.test.ts.selectAssetsBySelectedAccountGroup.Written by Cursor Bugbot for commit e6a0939. This will update automatically on new commits. Configure here.