Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

(Fix) remove module feature#1646

Merged
fernandomg merged 5 commits intodevelopmentfrom
fix/1367-remove-module
Nov 25, 2020
Merged

(Fix) remove module feature#1646
fernandomg merged 5 commits intodevelopmentfrom
fix/1367-remove-module

Conversation

@fernandomg
Copy link
Contributor

This PR closes #1367, by:

  • changing how the module pair is organized, now it's more natural: (prev, module) tuple
  • also added comments to better explain what's going on

- changed how the module pair is organized, now it's more natural (prev, module) tuple
@fernandomg fernandomg self-assigned this Nov 24, 2020
@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Nov 24, 2020

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 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@ghost
Copy link

ghost commented Nov 24, 2020

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

@ghost
Copy link

ghost commented Nov 24, 2020

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

@matextrem matextrem self-requested a review November 25, 2020 07:21
@ghost
Copy link

ghost commented Nov 25, 2020

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

@ghost
Copy link

ghost commented Nov 25, 2020

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

@francovenica
Copy link
Contributor

I gave it a try in a safe where I have 3 modules and I was having issues to remove it in other PR's.
I had no problems here, I was able to remove it just fine

image

const safeAddress = useSelector(safeParamAddressFromStateSelector)
const dispatch = useDispatch()

const explorerInfo = getExplorerInfo(selectedModule[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename selectedModule to selectedPairModule?
also, could it be possible to assign const prevModule = selectedModule[0]? I think it will help to understand what selectedModule[0] is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the assignment to a variable was already done, but I'm going with the selectedModulePair variable name

Copy link
Contributor

@nicosampler nicosampler left a comment

Choose a reason for hiding this comment

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

@fernandomg I left comments, please take a look at them.

@ghost
Copy link

ghost commented Nov 25, 2020

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

@ghost
Copy link

ghost commented Nov 25, 2020

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

@ghost
Copy link

ghost commented Nov 25, 2020

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

@ghost
Copy link

ghost commented Nov 25, 2020

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

@fernandomg fernandomg merged commit dca9f19 into development Nov 25, 2020
@fernandomg fernandomg deleted the fix/1367-remove-module branch November 25, 2020 18:35
@github-actions github-actions bot locked and limited conversation to collaborators Nov 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove module tx is always failed in the safe = 0x1230B3d59858296A31053C1b8562Ecf89A2f888b

5 participants