Refactor: chain features and versioned features #3120

katspaugh · 2021-12-06T12:01:41Z

What it solves

How this PR fixes it

Store both chain features and Safe version features in the enabledFeatures.
When checking if a feature is enabled, always use hasFeature which takes an optional Safe version param.

How to test it

Test optional safeTxGas for 1.3.0 Safes and that it's obligatory for 1.1.1
Test maxFeePerGas vs gasLimit on mainnet (maxFeePerGas) vs Polygon (gasLimit)

github-actions · 2021-12-06T12:01:58Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2021-12-06T12:06:22Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	6	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

iamacook

SAFE_TX_GAS_OPTIONAL does not exist on the FEATURES enum because I was lead to believe it was a Safe feature and therefore not chain related. Do we want to now add it?

iamacook · 2021-12-06T12:06:17Z

src/logic/safe/store/actions/__tests__/utils.test.ts

        FEATURES.SAFE_APPS,
        FEATURES.CONTRACT_INTERACTION,
-        SAFE_FEATURES.SAFE_TX_GAS_OPTIONAL,
+        FEATURES.SAFE_TX_GAS_OPTIONAL,


I thought this was a safe-specific feature? Hence, it wasn't included in the FEATURES enum in the gateway types.

That's correct but I see no harm in it being in either the enum or the store.

We can use this prior PR then.

Good point, reopened + added EIP1559.

coveralls · 2021-12-06T12:08:29Z

Pull Request Test Coverage Report for Build 1545378361

12 of 16 (75.0%) changed or added relevant lines in 5 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+1.4%) to 32.372%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/logic/safe/transactions/gas.ts	1	2	50.0%
src/components/AppLayout/Sidebar/useSidebarItems.tsx	0	3	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/config/index.ts	1	60.78%

Totals
Change from base Build 1545283506:	1.4%
Covered Lines:	3080
Relevant Lines:	8460

💛 - Coveralls

github-actions · 2021-12-06T13:36:50Z

Deployment links

🟠 Safe Rinkeby	⚪ Safe Mainnet	🟣 Safe Polygon	🟡 Safe BSC	⚫ Safe Arbitrum	🟢 Safe xDai

github-actions · 2021-12-06T13:45:51Z

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1545402418

Failed tests:

❌ Add an existing safe Add an existing safe
❌ Address book Address book
❌ Safe Apps List Safe Apps List
❌ Read-only transaction creation and review Create and review a Send Funds transaction

katspaugh · 2021-12-06T16:21:12Z

src/logic/safe/utils/safeVersion.ts

+const FEATURES_BY_VERSION: Record<string, string> = {
+  [FEATURES.ERC1155]: '>=1.1.1',
+  [FEATURES.SAFE_TX_GAS_OPTIONAL]: '>=1.3.0',


FEATURES_BY_VERSION is now a map of features to versions.

katspaugh · 2021-12-06T16:22:14Z

src/logic/safe/utils/safeVersion.ts

-  }
-  return featureConfig.validVersion ? semverSatisfies(version, featureConfig.validVersion) : true
+const isEnabledByVersion = (feature: FEATURES, version: string): boolean => {
+  if (!(feature in FEATURES_BY_VERSION)) return true


If the feature is in the chain features, but not in the versioned map, then we assume it's enabled for all Safe versions.

iamacook

Do you think the hasFeature function should move to /src/config?

katspaugh · 2021-12-06T16:24:27Z

But it depends on the Safe version.

francovenica · 2021-12-07T02:29:40Z

I'm not sure what "Test maxFeePerGas vs gasLimit on mainnet (maxFeePerGas) vs Polygon (gasLimit)" means. I tried tx in both networks and the MM popUp and the modals in the gnosis app looks the same in both networks

Issue:
Safes 1.0.0 still show collectibles.
Safe rinkeby 1.0.0: 0x5A7140f5DEabc7e211Da114081d9b0Fc4bf4cAf2

Question: Collectibles were enabled by safes 1.0.0 a while ago on purpose. Has something change that we have to disable the option for those users?

katspaugh · 2021-12-07T07:58:15Z

@francovenica you can see the difference between maxFeePerGas and gasLimit if you edit the gas price in MM. When using gasLimit, Max Base Fee and Max Priority Fee will (incorrectly) hold the same value.

Question: Collectibles were enabled by safes 1.0.0 a while ago on purpose. Has something change that we have to disable the option for those users?

Really? In the code it looked like Collectibles were deactivated for Safes prior to 1.1.1. If it's not the case on prod, we can get rid of that check entirely and enable Collectibles unconditionally for all Safes.

katspaugh · 2021-12-07T18:06:04Z

@francovenica has clarified that collectibles should indeed be activated for all versions.

Refactor: chain features and versioned features

ffdbc21

katspaugh requested a review from iamacook December 6, 2021 12:01

iamacook reviewed Dec 6, 2021

View reviewed changes

Update FEATURES type

cb15000

katspaugh added 2 commits December 6, 2021 16:15

Fix tests

d47fc2f

Pull from dev

2de4c92

katspaugh commented Dec 6, 2021

View reviewed changes

iamacook approved these changes Dec 6, 2021

View reviewed changes

katspaugh mentioned this pull request Dec 6, 2021

Options in the sidebar menu are missing #3118

Closed

katspaugh merged commit a2cd631 into dev Dec 7, 2021

katspaugh deleted the features branch December 7, 2021 10:05

github-actions bot locked and limited conversation to collaborators Dec 7, 2021

Refactor: chain features and versioned features #3120

Refactor: chain features and versioned features #3120

Uh oh!

Conversation

katspaugh commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it solves

How this PR fixes it

How to test it

Uh oh!

github-actions bot commented Dec 6, 2021

Uh oh!

github-actions bot commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

iamacook left a comment

Choose a reason for hiding this comment

Uh oh!

iamacook Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamacook Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1545378361

💛 - Coveralls

Uh oh!

github-actions bot commented Dec 6, 2021

Deployment links

Uh oh!

github-actions bot commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katspaugh Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

katspaugh Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

iamacook left a comment

Choose a reason for hiding this comment

Uh oh!

katspaugh commented Dec 6, 2021

Uh oh!

francovenica commented Dec 7, 2021

Uh oh!

katspaugh commented Dec 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

katspaugh commented Dec 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

katspaugh commented Dec 6, 2021 •

edited

Loading

github-actions bot commented Dec 6, 2021 •

edited

Loading

katspaugh Dec 6, 2021 •

edited

Loading

coveralls commented Dec 6, 2021 •

edited

Loading

github-actions bot commented Dec 6, 2021 •

edited

Loading

katspaugh commented Dec 7, 2021 •

edited

Loading