(Fix) Modules not shown in Advanced Settings by fernandomg · Pull Request #1516 · 5afe/safe-react

fernandomg · 2020-10-21T21:32:09Z

This PR closes #1494, by changing the way modules information is retrieved.

The error: was in assuming that getModulesPaginated was present in all Safe's versions, but it wasn't implemented until v1.1.1.

The fix changes the strategy by using the SafeInfo (safes/{safeAddress}) transactions service endpoint, which returns modules information and more.

The only restriction that the transaction service has is that it's using getModules method, which in versions >=v1.1.1 will return up to 10 modules.

So, if we face that scenario, we'll be querying getModulesPaginated as a last resource.

More info at https://github.com/gnosis/safe-react/blob/fix/1494-modules-not-shown/src/logic/safe/utils/modules.ts#L40

- also, reused the recently created method to avoid repetitions for the following SafeServiceUri-dependant URIs

github-actions · 2020-10-21T21:32:26Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2020-10-21T21:36:59Z

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	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

ghost · 2020-10-21T21:46:23Z

Travis automatic deployment:
https://pr1516--safereact.review.gnosisdev.com/app

src/config/index.ts

ghost · 2020-10-23T00:26:56Z

Travis automatic deployment:
https://pr1516--safereact.review.gnosisdev.com/app

src/logic/safe/utils/buildSafeInformationUrl.ts

src/logic/safe/utils/safeInformation.ts

nicosampler · 2020-10-23T14:24:04Z

src/logic/safe/utils/modules.ts

+ * @returns Array<ModulePair> | null | undefined
+ */
+export const getModules = async (safeInfo: SafeInfo | void): Promise<Array<ModulePair> | null | undefined> => {
+  if (safeInfo) {


can you inverse the condition? if (!safeInfo) that will remove 1 nested level

nicosampler · 2020-10-23T15:41:20Z

@fernandomg there are some conflicts

# Conflicts: # src/config/index.ts # src/logic/currencyValues/api/fetchTokenCurrenciesBalances.ts

ghost · 2020-10-23T16:08:08Z

Travis automatic deployment:
https://pr1516--safereact.review.gnosisdev.com/app

ghost · 2020-10-26T11:37:32Z

Travis automatic deployment:
https://pr1516--safereact.review.gnosisdev.com/app

ghost · 2020-10-26T15:11:24Z

Travis automatic deployment:
https://pr1516--safereact.review.gnosisdev.com/app

francovenica · 2020-10-26T20:41:06Z

Tried in a safe 1.0.0

Safe: https://pr1516--safereact.review.gnosisdev.com/app/#/safes/0xeca13481F8514752dC08106c7A265c85caFa563E/settings
In the dev env:

After the fix, in the PR:

francovenica · 2020-10-26T21:00:47Z

There is a issue with tx not being able to be executed when a tx is "off chain", it fails on its first signature.
With Fer we were checking what it was and it seems there is an "/" missing in the URL where the post is being done.

- 310 made POST requests fail

fernandomg · 2020-10-26T21:28:25Z

There is a issue with tx not being able to be executed when a tx is "off chain", it fails on its first signature.
With Fer we were checking what it was and it seems there is an "/" missing in the URL where the post is being done.

@francovenica fixed, thanks so much for the catch!

ghost · 2020-10-26T21:40:23Z

Travis automatic deployment:
https://pr1516--safereact.review.gnosisdev.com/app

matextrem

LGTM👍

francovenica · 2020-10-27T13:47:14Z

Tried again In this safe https://pr1516--safereact.review.gnosisdev.com/app/#/safes/0x9913B9180C20C6b0F21B6480c84422F6ebc4B808/transactions

I was able to do a regular "Send Funds" tx and add the module that Tobias pointed out in the issue ticket.

Looks good to me

fernandomg added 6 commits October 21, 2020 18:09

add getSafeServiceUri method in configs

0610353

- also, reused the recently created method to avoid repetitions for the following SafeServiceUri-dependant URIs

create util files to retrieve safeInfo from safe-transactions service

c227665

create util file to handle Safe's modules logic in one place

0dc91ed

refactor how we fetch Safe's modules information

d2cbdfc

use module service method to remove module

e790738

decouple modules data retrieval logic from getSafeInfo

8660d09

fernandomg requested review from Agupane, dasanra, matextrem and nicosampler October 21, 2020 21:32

fernandomg self-assigned this Oct 21, 2020

fernandomg changed the title ~~(Fix) Modules not shown~~ (Fix) Modules not shown in Advanced Settings Oct 21, 2020

dasanra reviewed Oct 22, 2020

View reviewed changes

src/config/index.ts Outdated Show resolved Hide resolved

fernandomg added 2 commits October 22, 2020 19:21

Merge branch 'development' into fix/1494-modules-not-shown

c035c9d

unify gnosis service url retrieval

cc3e840

fernandomg requested a review from dasanra October 23, 2020 00:13

nicosampler reviewed Oct 23, 2020

View reviewed changes

src/logic/safe/utils/buildSafeInformationUrl.ts Show resolved Hide resolved

nicosampler reviewed Oct 23, 2020

View reviewed changes

src/logic/safe/utils/safeInformation.ts Show resolved Hide resolved

nicosampler reviewed Oct 23, 2020

View reviewed changes

revert if condition for safeInfo

9a0fabe

fernandomg requested a review from nicosampler October 23, 2020 15:22

nicosampler approved these changes Oct 23, 2020

View reviewed changes

Merge branch 'development' into fix/1494-modules-not-shown

949e0b3

# Conflicts: # src/config/index.ts # src/logic/currencyValues/api/fetchTokenCurrenciesBalances.ts

Merge branch 'development' into fix/1494-modules-not-shown

ea91798

Merge branch 'development' into fix/1494-modules-not-shown

cafb888

dasanra approved these changes Oct 26, 2020

View reviewed changes

add trailing slash to gnosis services URLs to avoid HTTP 301 redirects

b2c8d19

- 310 made POST requests fail

matextrem approved these changes Oct 27, 2020

View reviewed changes

francovenica self-requested a review October 27, 2020 13:47

francovenica approved these changes Oct 27, 2020

View reviewed changes

fernandomg merged commit aa181fb into development Oct 27, 2020

fernandomg deleted the fix/1494-modules-not-shown branch October 27, 2020 14:04

github-actions bot locked and limited conversation to collaborators Oct 27, 2020

Conversation

fernandomg commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 21, 2020

Uh oh!

github-actions bot commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ESLint Summary View Full Report

Uh oh!

ghost commented Oct 21, 2020

Uh oh!

Uh oh!

ghost commented Oct 23, 2020

Uh oh!

Uh oh!

Uh oh!

nicosampler Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

fernandomg Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

nicosampler commented Oct 23, 2020

Uh oh!

ghost commented Oct 23, 2020

Uh oh!

ghost commented Oct 26, 2020

Uh oh!

ghost commented Oct 26, 2020

Uh oh!

francovenica commented Oct 26, 2020

Uh oh!

francovenica commented Oct 26, 2020

Uh oh!

fernandomg commented Oct 26, 2020

Uh oh!

ghost commented Oct 26, 2020

Uh oh!

matextrem left a comment

Choose a reason for hiding this comment

Uh oh!

francovenica commented Oct 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fernandomg commented Oct 21, 2020 •

edited

Loading

github-actions bot commented Oct 21, 2020 •

edited

Loading