Skip to content

Conversation

@infiniteflower
Copy link
Contributor

@infiniteflower infiniteflower commented Apr 30, 2024

This PR implements the changes found in branch patch/mobile-swaps-v6.9.3-stx, which was a patch for Mobile

It does the following:

  1. Adds fetchSwapsFeatureFlags as a util function which allows Mobile to fetch global and chainId specific feature flags all at once.
  2. Adds relevant tests

@infiniteflower infiniteflower requested a review from dan437 as a code owner April 30, 2024 17:17
@infiniteflower infiniteflower requested a review from a team April 30, 2024 17:17
@nikoferro
Copy link

@infiniteflower just to double check, this does not rely in any way on previous patches, is that correct?

asking this since we are removing current ones on MetaMask/metamask-mobile#9286

extension_active: boolean;
fallback_to_v1?: boolean;
fallback_to_v1: boolean;
fallbackToV1: boolean;
Copy link
Contributor

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

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

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

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

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

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

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

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.

@infiniteflower
Copy link
Contributor Author

@infiniteflower just to double check, this does not rely in any way on previous patches, is that correct?

asking this since we are removing current ones on MetaMask/metamask-mobile#9286

It should be fine, it hits the same endpoint but just doesn't modify the endpoint results like the other function (fetchSwapsFeatureLiveness)

@infiniteflower infiniteflower requested a review from legobeat May 9, 2024 16:23
…s and use new interfaces for fetchSwapsFeatureFlags
@infiniteflower infiniteflower requested a review from nikoferro May 10, 2024 16:46
@infiniteflower infiniteflower merged commit d2ea3fb into main May 14, 2024
@infiniteflower infiniteflower deleted the feat/stx-mobile-fetchSwapsFeatureFlags branch May 14, 2024 14:10
infiniteflower added a commit to MetaMask/metamask-mobile that referenced this pull request May 16, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

This PR only covers a smaller subset of the files from #9565 for ease of
reviewing. Please refer to that issue for videos. This PR is mostly for
Engine and the views for Smart Transactions (STX). Merging this PR will
cause changes to the functionality and behavior of the app, effectively
making the feature "live".

It covers the following areas:
1. `app/components`
2. `app/core`
3. `app/reducers`
4. `app/selectors`
5. `app/util/test`
6. `patches`
- `PreferencesController` ([PR
here](MetaMask/core#3815) to apply update to
`main`)
- `SwapsController` ([PR
here](MetaMask/swaps-controller#229) to apply
updates to `main`)

Also fixed an issue for network onboarding where it would never set
onboarded to `true`. This needed to be fixed so we could properly show
the STX Opt In Modal. We only show the Opt In modal if the user has
onboarded onto the network, to prevent multiple modals from showing up
at the same time. See discussion
#9448 (comment)

## **Related issues**

Fixes: n/a
- Targeting: #9442
- Broken out from: #9565

## **Manual testing steps**

Refer to #9565

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

Refer to #9565

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: tommasini <46944231+tommasini@users.noreply.github.com>
Co-authored-by: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com>
Andepande pushed a commit to MetaMask/metamask-mobile that referenced this pull request May 21, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

This PR only covers a smaller subset of the files from #9565 for ease of
reviewing. Please refer to that issue for videos. This PR is mostly for
Engine and the views for Smart Transactions (STX). Merging this PR will
cause changes to the functionality and behavior of the app, effectively
making the feature "live".

It covers the following areas:
1. `app/components`
2. `app/core`
3. `app/reducers`
4. `app/selectors`
5. `app/util/test`
6. `patches`
- `PreferencesController` ([PR
here](MetaMask/core#3815) to apply update to
`main`)
- `SwapsController` ([PR
here](MetaMask/swaps-controller#229) to apply
updates to `main`)

Also fixed an issue for network onboarding where it would never set
onboarded to `true`. This needed to be fixed so we could properly show
the STX Opt In Modal. We only show the Opt In modal if the user has
onboarded onto the network, to prevent multiple modals from showing up
at the same time. See discussion
#9448 (comment)

## **Related issues**

Fixes: n/a
- Targeting: #9442
- Broken out from: #9565

## **Manual testing steps**

Refer to #9565

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

Refer to #9565

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: tommasini <46944231+tommasini@users.noreply.github.com>
Co-authored-by: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants