Skip to content

feat: add util to fetch global and chain feature flags at once #229

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.

Sign up for GitHub

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

Merged
infiniteflower merged 8 commits into from
May 14, 2024
Merged

feat: add util to fetch global and chain feature flags at once #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c8aeffb
feat: add util to fetch global and chain feature flags at once
infiniteflower Apr 30, 2024
0577f20
fix: lint errors
infiniteflower Apr 30, 2024
e5aabef
fix: lint errors
infiniteflower Apr 30, 2024
fe21b10
fix: wrong test
infiniteflower Apr 30, 2024
6c37292
Merge branch 'main' into feat/stx-mobile-fetchSwapsFeatureFlags
legobeat May 9, 2024
34c4794
fix: make fallback_to_v1 optional to avoid breaking change
infiniteflower May 9, 2024
6d7bfa7
chore: improve test names
infiniteflower May 9, 2024
71f40f2
chore: revert back to original interface for fetchSwapsFeatureLivenes…
infiniteflower May 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/swapsInterfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,42 @@ export type NetworksFeatureStatus = {
[network: string]: NetworkFeatureFlags;
};

export type NetworkFeatureFlagsAll = {
mobile_active: boolean;
extension_active: boolean;
fallback_to_v1?: boolean;
fallbackToV1: boolean;
Copy link
Contributor

@legobeat legobeat May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest making these snake_case for consistency and the changing all of them (including the _active flags above) to camelCase in a follow-up PR.
That way:

  • This PR can be kept non-breaking and scoped
  • Consistency is kept
  • No need for duplicate flags (fallback_to_v1 plus fallBackToV1)
  • Eventually this will conform to the global style and use camelCase

Copy link
Contributor Author

@infiniteflower infiniteflower May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just dumping all of the raw feature flags from the endpoint directly so that's why there's all these sort of overlapping values.

Copy link
Contributor

@legobeat legobeat May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. So maybe it makes sense to make two types: One to interface with that API (using something like here) and one used internally and everywhere else (using a consistent, internal API). The consumption of the API would then involve parsing one to the other. Does that make sense?

Copy link
Contributor Author

@infiniteflower infiniteflower May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea makes sense, I'll adjust the types so there's no breaking changes.

To provide some context around this function, I just wanted to give clients the ability to consume the raw API directly, otherwise it necessitates we would have to update to the SwapsController if things fall outside of the current utils function such as in the STX case where the STX feature flags are outside of any chain specific feature flags.

So to just give the clients more flexibility, this util function just gives the raw endpoint result back to the client.

Copy link
Contributor Author

@infiniteflower infiniteflower May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I suggest refactoring the API in another PR as it would mean we need to refactor the clients as well. I just need fetchSwapsFeatureFlags to accommodate Smart Transactions.

My type updates are simply reflecting the current state of the SwapsController return values. Without the change to:

export type NetworkFeatureFlags = {
  mobile_active: boolean;
  extension_active: boolean;
  fallback_to_v1?: boolean;
  fallbackToV1: boolean;
  mobileActive: boolean;
  extensionActive: boolean;
  mobileActiveIOS: boolean;
  mobileActiveAndroid: boolean;

  smartTransactions: {
    expectedDeadline: number;
    maxDeadline: number;
    returnTxHashAsap: boolean;
  };
};

The return values from fetchSwapsFeatureLiveness are not properly typed.

Copy link
Contributor

@legobeat legobeat May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here would be precisely to make the clients-facing API more stable. So in this first step, we can just stick to the existing interface, extended with flags necessary for the purpose of the functionality in this PR.

Then the types used to interface with the server API would be what you write here.

And any translation necessary (e.g. case-insensivity, supporting two different spellings of the same value, or response validation/error handling) could be handled in fetchSwapsFeatureLiveness.

This way:

  • No breaking changes needed
  • Future changes in controller<>client API does not need to involve server
  • Future changes in server<>controller API does not need to involve client
  • The API presented from this controller can do better despite "The return values from fetchSwapsFeatureLiveness are not properly typed."
  • There is a clear path towards cleaning it up further down the line

Copy link
Contributor

@legobeat legobeat May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If more flexibility and decoupling is desired for use of the data that is actually unrelated to the swaps controller itself, maybe the currently static feature flags could be made into optional runtime configuration, with the existing ones as default?

Copy link
Contributor Author

@infiniteflower infiniteflower May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I've reverted the interface for NetworkFeatureFlags and NetworksFeatureStatus for fetchSwapsFeatureLiveness back to their original states.

I've added NetworkFeatureFlagsAll and NetworksFeatureStatusAll for use with fetchSwapsFeatureFlags, which would basically be the raw dump of the API endpoint's result.

So nothing will change for existing methods. This PR only adds new a new method (fetchSwapsFeatureFlags) and new interfaces (NetworkFeatureFlagsAll and NetworksFeatureStatusAll) that only apply to that new method.

mobileActive: boolean;
extensionActive: boolean;
mobileActiveIOS: boolean;
mobileActiveAndroid: boolean;

smartTransactions: {
expectedDeadline: number;
maxDeadline: number;
returnTxHashAsap: boolean;
};
};

export type NetworksFeatureStatusAll = {
[network: string]: NetworkFeatureFlagsAll;
};

export type GlobalFeatureFlags = {
smart_transactions: {
mobile_active: boolean;
extension_active: boolean;
};
smartTransactions: {
mobileActive: boolean;
extensionActive: boolean;
mobileActiveIOS: boolean;
mobileActiveAndroid: boolean;
};
};

export type FeatureFlags = NetworksFeatureStatusAll & GlobalFeatureFlags;

/**
* Metadata needed to fetch quotes
* @interface APIFetchQuotesMetadata
Expand Down
100 changes: 100 additions & 0 deletions src/swapsUtil.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,106 @@ describe('SwapsUtil', () => {
});
});

describe('fetchSwapsFeatureFlags', () => {
it('should return network and global feature flags', async () => {
const featureFlags = {
bsc: {
mobile_active: false,
extension_active: true,
fallback_to_v1: true,
},
ethereum: {
mobile_active: false,
extension_active: true,
fallback_to_v1: true,
},
polygon: {
mobile_active: false,
extension_active: true,
fallback_to_v1: false,
},
smart_transactions: {
mobile_active: true,
extension_active: true
},
smartTransactions: {
mobileActive: true,
extensionActive: true,
mobileActiveIOS: false,
mobileActiveAndroid: false
},
swapRedesign: {
mobileActive: false,
extensionActive: true
}
}

mockFetch({
'https://swap.metaswap.codefi.network/featureFlags': {
body: featureFlags,
},
});
const featureLiveness = await swapsUtil.fetchSwapsFeatureFlags('0x1');
expect(featureLiveness).toEqual(featureFlags);
});

it('should return network and global feature flags regardless of unsupported networks', async () => {
const featureFlags = {
bsc: {
mobile_active: false,
extension_active: true,
fallback_to_v1: true,
},
ethereum: {
mobile_active: false,
extension_active: true,
fallback_to_v1: true,
},
polygon: {
mobile_active: false,
extension_active: true,
fallback_to_v1: false,
},
smart_transactions: {
mobile_active: true,
extension_active: true
},
smartTransactions: {
mobileActive: true,
extensionActive: true,
mobileActiveIOS: false,
mobileActiveAndroid: false
},
swapRedesign: {
mobileActive: false,
extensionActive: true
}
}

mockFetch({
'https://swap.metaswap.codefi.network/featureFlags': {
body: featureFlags,
},
});
const featureLiveness = await swapsUtil.fetchSwapsFeatureFlags(
'0x321',
);
expect(featureLiveness).toEqual(featureFlags);
});

it('should throw on exception', async () => {
mockFetch({
'https://swap.metaswap.codefi.network/featureFlags': {
throws: true,
},
});

await expect(async () =>
swapsUtil.fetchSwapsFeatureFlags('0x1'),
).rejects.toThrow();
});
});

describe('fetchGasPrices', () => {
it('should work', async () => {
mockFetch({
Expand Down
23 changes: 21 additions & 2 deletions src/swapsUtil.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
import type {
APIAggregatorMetadata,
APIFetchQuotesParams,
FeatureFlags,
NetworkFeatureFlags,
NetworksFeatureStatus,
Quote,
Expand Down Expand Up @@ -367,10 +368,10 @@ export async function fetchTopAssets(
}

/**
* Fetches feature flags from API URL.
* Fetches chainId specific feature flags from API URL.
* @param chainId - Current chainId.
* @param clientId - Client id.
* @returns Promise resolving to an object containing feature flags.
* @returns Promise resolving to an object containing feature flags for the chainId.
*/
export async function fetchSwapsFeatureLiveness(
chainId: Hex,
Expand All @@ -384,6 +385,24 @@ export async function fetchSwapsFeatureLiveness(
return status[networkName];
}

/**
* Fetches global and chainId specific feature flags from API URL.
* @param chainId - Current chainId.
* @param clientId - Client id.
* @returns Promise resolving to an object containing global and chainId specific feature flags.
*/
export async function fetchSwapsFeatureFlags(
chainId: Hex,
clientId?: string,
): Promise<FeatureFlags | undefined> {
const status: FeatureFlags = await handleFetch(
getBaseApiURL(APIType.FEATURE_FLAG, chainId),
{ method: 'GET', headers: getClientIdHeader(clientId) },
);

return status;
}

/**
* Fetches gas prices from API URL.
* @param chainId - Current chainId.
Expand Down