fix: use same feature for ofc tokens #6702
Conversation
mukeshsp
force-pushed
the
fix-ofc-features
branch
2 times, most recently
from
fe40ced to
7532524
Compare
mukeshsp
force-pushed
the
fix-ofc-features
branch
from
7532524 to
f589bb9
Compare
that is flaky test which is not related to my change |
mukeshsp
force-pushed
the
fix-ofc-features
branch
from
f589bb9 to
c10cb50
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mukeshsp
force-pushed
the
fix-ofc-features
branch
from
c10cb50 to
aa34a38
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the OFC (off-chain) coin feature system to ensure that OFC tokens inherit features from their corresponding native coins, specifically focusing on custody-related features. The changes implement a mechanism to dynamically filter and apply features from base coins to their OFC counterparts.
- Implements a feature inheritance system for OFC tokens to match native coin features
- Adds test coverage to validate feature parity between OFC and native coins
- Updates existing test logic to handle specific edge cases for CELO-based coins
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| modules/statics/src/ofc.ts | Adds getFilteredFeatures function and applies feature inheritance to all OFC token creation functions |
| modules/statics/test/unit/ofcCoinParity.ts | Adds new test to verify OFC tokens have matching custody features with their base coins |
| modules/statics/test/unit/coins.ts | Updates custody feature test to exclude specific CELO OFC variants and removes non-SD coin feature test |
| modules/statics/src/networkFeatureMapForTokens.ts | Changes OFC network feature mapping from using default features to an empty array |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
modules/statics/src/ofc.ts
Outdated
| @@ -1,5 +1,7 @@ | |||
| import { BaseCoin, BaseUnit, CoinFeature, CoinKind, KeyCurve, UnderlyingAsset } from './base'; | |||
| import { BaseNetwork, Networks, OfcNetwork } from './networks'; | |||
| import { allCoinsAndTokens } from './allCoinsAndToken'; | |||
There was a problem hiding this comment.
The import path './allCoinsAndToken' appears to be missing an 's' at the end. It should likely be './allCoinsAndTokens' to match standard naming conventions.
| import { allCoinsAndTokens } from './allCoinsAndToken'; | |
| import { allCoinsAndTokens } from './allCoinsAndTokens'; |
Copilot uses AI. Check for mistakes.
| const ofcFeatureSet = new Set(ofcFeatures); | ||
| const baseFeatureSet = new Set(baseFeatures); | ||
| const missingFeatures = Array.from(baseFeatureSet).filter( | ||
| (feature) => !ofcFeatureSet.has(feature) && feature.startsWith('custody') |
There was a problem hiding this comment.
The filter condition feature.startsWith('custody') is hardcoded and may miss other custody-related features that don't follow this naming convention. Consider using a more robust way to identify custody features, such as checking against a predefined list of custody feature constants.
| (feature) => !ofcFeatureSet.has(feature) && feature.startsWith('custody') | |
| (feature) => !ofcFeatureSet.has(feature) && CUSTODY_FEATURES.has(feature) |
Copilot uses AI. Check for mistakes.
| if (!allCoinsAndTokensMap) { | ||
| allCoinsAndTokensMap = CoinMap.fromCoins(allCoinsAndTokens); | ||
| } | ||
| return allCoinsAndTokensMap; |
There was a problem hiding this comment.
The lazy initialization pattern with a module-level variable could lead to issues in testing environments where the module might be reloaded. Consider implementing a more robust singleton pattern or making this function pure by accepting the coins map as a parameter.
| return allCoinsAndTokensMap; | |
| // Removed module-level cache to avoid issues with module reloads in testing environments. | |
| const getAllCoinsAndTokensMap = (): CoinMap => { | |
| return CoinMap.fromCoins(allCoinsAndTokens); |
Copilot uses AI. Check for mistakes.
mukeshsp
force-pushed
the
fix-ofc-features
branch
2 times, most recently
from
6b23a5c to
63719d6
Compare
mukeshsp
force-pushed
the
fix-ofc-features
branch
from
63719d6 to
b0c5619
Compare
There was a problem hiding this comment.
Seems like this should rather be named someCoinsAndTokens 😉
What's the logic that determines which ones are defined here and which ones in coins.ts?
There was a problem hiding this comment.
all the native coins and token will be in [allCoinsAndTokens.ts] and ofc tokens are in ofc.ts file
this is a flaky test which is failing for many PRs and we are looking into it parallelly |
We observed that separate features are being maintained for OFC tokens, but they should use the same features as their corresponding native coin or token. I confirmed with the Go team (with Manav) that currently, the OFC feature from the SDK isn’t being used—instead, they have a hardcoded mapping in the Prime service.
Problem:
Currently, the mapping of OFC tokens with Trust is based on hardcoded data in the Prime service. This means that whenever a new token needs to be onboarded, the hardcoded file has to be updated, which slows down the onboarding process.
Solution:
Instead of using hardcoded mappings, we should leverage the feature flags defined for OFC tokens. To enable this, we first need to correct the feature flags for OFC tokens.
Ticket: WIN-6707