feat(positions): multichain positions #5093
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
|
Looks like there's a schema update needed and some tests that also need to be updated in:
I also see one case where we use |
cajubelt
changed the title
1 build had no size change
Celo (test) 1.82.0 (147)
|
cajubelt
added a commit
to valora-inc/hooks
that referenced
this pull request
Apr 2, 2024
… and ethereum (#349) Changes made: - updates the api to use Valora's standard network ID's for multichain requests - updates curve hook to actually return some data for multichain requests - seals off all other hooks with comments indicating whether they can be extended or are specific to Celo - uses blockchain api endpoint to get token info (which returns multichain token metadata and prices, rather than just celo token metadata and prices) - refactors to use Valora's standard for token ID's instead of addresses as token identifiers, to work for positions that include native assets lacking a contract address, like ETH BREAKING CHANGE: 'network' becomes 'networkId' in a few places in the API response. Requests can still use legacy 'network' request parameter, with either 'celo' or 'celoAlfajores' recognized. The wallet is updated in [this PR](valora-inc/wallet#5093) to adapt to the change, but old versions should continue to work without changes, since the 'network' request is not used for anything critical (see description on [wallet PR](valora-inc/wallet#5093) for details) test plan: - [x] **Backwards compatibility test**: run manual e2e test with 'main' branch of wallet (which still uses 'network' in requests and expects 'network' in response data) to verify that positions are queried for and displayed correctly for legacy app versions, e.g. that the changes are backwards compatible enough not to break anything. use local hooks endpoint, local wallet build, and hardcoded wallet address in the request.   - [x] hunt thru wallet code to make sure the legacy 'network' field on some response data is not used for anything important - [x] **Multichain functionality test**: run manual e2e test with `cajubelt/multichain-positions` branch of wallet, with an account that has a curve position on ethereum. use local hooks endpoint, local wallet build, and hardcoded network id in the request.  - [x] update existing unit and e2e tests to add coverage for curve on ethereum. sent the address used in e2e tests a curve lp token. cc https://linear.app/valora/issue/RET-941/%5Bhooks%5D-multichain-support
cajubelt
changed the title
cajubelt
commented
Apr 4, 2024
src/analytics/Properties.tsx
Outdated
There was a problem hiding this comment.
for analytics types I kept the "network" key the same, to avoid adding a column to BQ tables. for application code I migrated to "networkId" for consistency with other features
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5093 +/- ##
=======================================
Coverage 85.78% 85.78%
=======================================
Files 744 744
Lines 30348 30382 +34
Branches 5238 5244 +6
=======================================
+ Hits 26033 26064 +31
- Misses 4076 4079 +3
Partials 239 239
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
cajubelt
changed the title
silasbw
approved these changes
Apr 5, 2024
cajubelt
commented
Apr 8, 2024
| @@ -74,6 +74,7 @@ const storeWithPositions = { | |||
| ...storeWithTokenBalances, | |||
| positions: { | |||
| positions: mockPositions, // Total value of positions is ~$7.91 or ~₱10.52 | |||
| shortcuts: [], | |||
There was a problem hiding this comment.
previously, the mock schema had an empty list in this field, which wasn't ideal imo because it didn't guarantee that we had compile-time checks for the mock data. when I updated the "root" state mock to include a nonemtpy list of shortcuts, I needed this to simulate a store that did not have any claimable positions
| } | ||
| type UpdatedToken = { networkId: NetworkId; tokens?: UpdatedToken[] } | ||
|
|
||
| function recursivelyUpdateToken(token: LegacyToken): UpdatedToken { |
There was a problem hiding this comment.
the mock data includes a nested token, so we have test coverage for this
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
shottah
pushed a commit
to zed-io/kolektivo
that referenced
this pull request
May 15, 2024
### Description gets positions and shortcuts for whichever networks are enabled in our dynamic config, rather than just celo relies on valora-inc/hooks#362 ### Test plan - ci - manual test with local hooks api on wallet with both ethereum and celo positions (also unearthed a redux migration bug with this! hooray for testing!) ### Related issues fixes https://linear.app/valora/issue/RET-941/%5Bhooks%5D-multichain-support ### Backwards compatibility there is only one place where the new API will affect application code for old app versions, which is in the `keyExtractor` function in `AssetList.tsx`. The function returns the following for positions: `${activeTab}-${item.appId}-${item.networkId}-${item.address}-${index}`. I think this is ok, since old app versions only get celo positions right now, so when `item.networkId` evaluates to `undefined` for them, it shouldn't cause collisions with other positions on other networks. the other changes are all in analytics events, which should be something we can live with ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [x] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
gets positions and shortcuts for whichever networks are enabled in our dynamic config, rather than just celo
relies on valora-inc/hooks#362
Test plan
Related issues
fixes https://linear.app/valora/issue/RET-941/%5Bhooks%5D-multichain-support
Backwards compatibility
there is only one place where the new API will affect application code for old app versions, which is in the
keyExtractorfunction inAssetList.tsx. The function returns the following for positions:${activeTab}-${item.appId}-${item.networkId}-${item.address}-${index}. I think this is ok, since old app versions only get celo positions right now, so whenitem.networkIdevaluates toundefinedfor them, it shouldn't cause collisions with other positions on other networks.the other changes are all in analytics events, which should be something we can live with
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: