From dca9f19fd0b55cc7d16f4e4fab0992d3268c8c6f Mon Sep 17 00:00:00 2001 From: Fernando Date: Wed, 25 Nov 2020 15:35:09 -0300 Subject: [PATCH] (Fix) remove module feature (#1646) --- src/logic/safe/store/models/safe.ts | 7 +- .../safe/utils/__tests__/modules.test.ts | 111 ++++++++++++++++++ src/logic/safe/utils/modules.ts | 35 +++++- .../Settings/Advanced/ModulesTable.tsx | 15 +-- .../Settings/Advanced/RemoveModuleModal.tsx | 17 +-- 5 files changed, 163 insertions(+), 22 deletions(-) create mode 100644 src/logic/safe/utils/__tests__/modules.test.ts diff --git a/src/logic/safe/store/models/safe.ts b/src/logic/safe/store/models/safe.ts index ef217e28..89515c76 100644 --- a/src/logic/safe/store/models/safe.ts +++ b/src/logic/safe/store/models/safe.ts @@ -6,7 +6,12 @@ export type SafeOwner = { address: string } -export type ModulePair = [string, string] +export type ModulePair = [ + // previous module + string, + // module + string, +] export type SafeRecordProps = { name: string diff --git a/src/logic/safe/utils/__tests__/modules.test.ts b/src/logic/safe/utils/__tests__/modules.test.ts new file mode 100644 index 00000000..ccaf4b25 --- /dev/null +++ b/src/logic/safe/utils/__tests__/modules.test.ts @@ -0,0 +1,111 @@ +import { SENTINEL_ADDRESS } from 'src/logic/contracts/safeContracts' +import { buildModulesLinkedList } from 'src/logic/safe/utils/modules' + +describe('modules -> buildModulesLinkedList', () => { + let moduleManager + + beforeEach(() => { + moduleManager = { + modules: { + [SENTINEL_ADDRESS]: SENTINEL_ADDRESS, + }, + enableModule: function (module: string) { + this.modules[module] = this.modules[SENTINEL_ADDRESS] + this.modules[SENTINEL_ADDRESS] = module + }, + disableModule: function (prevModule: string, module: string) { + this.modules[prevModule] = this.modules[module] + this.modules[module] = '0x0' + }, + getModules: function (): string[] { + const modules: string[] = [] + let module: string = this.modules[SENTINEL_ADDRESS] + + while (module !== SENTINEL_ADDRESS) { + modules.push(module) + module = this.modules[module] + } + + return modules + }, + } + }) + + it(`should build a collection of addresses pair associated to a linked list`, () => { + // Given + const listOfModules = ['0xa', '0xb', '0xc', '0xd', '0xe', '0xf'] + + // When + const modulesPairList = buildModulesLinkedList(listOfModules) + + // Then + expect(modulesPairList).toStrictEqual([ + [SENTINEL_ADDRESS, '0xa'], + ['0xa', '0xb'], + ['0xb', '0xc'], + ['0xc', '0xd'], + ['0xd', '0xe'], + ['0xe', '0xf'], + ]) + }) + + it(`should properly provide a list of modules pair to remove an specified module`, () => { + // Given + moduleManager.enableModule('0xc') + moduleManager.enableModule('0xb') + moduleManager.enableModule('0xa') // returned list is ordered [0xa, 0xb, 0xc] + const modulesPairList = buildModulesLinkedList(moduleManager.getModules()) + + // When + const moduleBPair = modulesPairList?.[1] ?? [] + moduleManager.disableModule(...moduleBPair) + + // Then + expect(moduleManager.modules['0xb']).toBe('0x0') + expect(moduleManager.getModules()).toStrictEqual(['0xa', '0xc']) + expect(buildModulesLinkedList(moduleManager.getModules())).toStrictEqual([ + [SENTINEL_ADDRESS, '0xa'], + ['0xa', '0xc'], + ]) + }) + + it(`should properly provide a list of modules pair to remove the firstly added module`, () => { + // Given + moduleManager.enableModule('0xc') + moduleManager.enableModule('0xb') + moduleManager.enableModule('0xa') // returned list is ordered [0xa, 0xb, 0xc] + const modulesPairList = buildModulesLinkedList(moduleManager.getModules()) + + // When + const moduleBPair = modulesPairList?.[2] ?? [] + moduleManager.disableModule(...moduleBPair) + + // Then + expect(moduleManager.modules['0xc']).toBe('0x0') + expect(moduleManager.getModules()).toStrictEqual(['0xa', '0xb']) + expect(buildModulesLinkedList(moduleManager.getModules())).toStrictEqual([ + [SENTINEL_ADDRESS, '0xa'], + ['0xa', '0xb'], + ]) + }) + + it(`should properly provide a list of modules pair to remove the lastly added module`, () => { + // Given + moduleManager.enableModule('0xc') + moduleManager.enableModule('0xb') + moduleManager.enableModule('0xa') // returned list is ordered [0xa, 0xb, 0xc] + const modulesPairList = buildModulesLinkedList(moduleManager.getModules()) + + // When + const moduleBPair = modulesPairList?.[0] ?? [] + moduleManager.disableModule(...moduleBPair) + + // Then + expect(moduleManager.modules['0xa']).toBe('0x0') + expect(moduleManager.getModules()).toStrictEqual(['0xb', '0xc']) + expect(buildModulesLinkedList(moduleManager.getModules())).toStrictEqual([ + [SENTINEL_ADDRESS, '0xb'], + ['0xb', '0xc'], + ]) + }) +}) diff --git a/src/logic/safe/utils/modules.ts b/src/logic/safe/utils/modules.ts index b15e2c15..7b934609 100644 --- a/src/logic/safe/utils/modules.ts +++ b/src/logic/safe/utils/modules.ts @@ -9,11 +9,32 @@ type ModulesPaginated = { next: string } -const buildModulesLinkedList = (modules: string[], nextModule: string = SENTINEL_ADDRESS): Array | null => { +/** + * Builds a collection of tuples with (prev, module) module addresses + * + * The `modules` param, is organized from the most recently added to the oldest. + * + * By assuming this, we are able to recreate the linked list that's defined at contract level + * considering `0x1` (SENTINEL_ADDRESS) address as the list's initial node. + * + * Given this scenario, we have a linked list in the form of + * + * **`0x1->modules[n]->module[n-1]->module[0]->0x1`** + * + * So, + * - if we want to disable `module[n]`, we need to pass `(module[n], 0x1)` as arguments, + * - if we want to disable `module[n-1]`, we need to pass `(module[n-1], module[n])`, + * - ... and so on + * @param {Array} modules + * @returns null | Array + */ +export const buildModulesLinkedList = (modules: string[]): Array | null => { if (modules?.length) { return modules.map((moduleAddress, index, modules) => { - const prevModule = modules[index + 1] - return [moduleAddress, prevModule !== undefined ? prevModule : nextModule] + if (index === 0) { + return [SENTINEL_ADDRESS, moduleAddress] + } + return [modules[index - 1], moduleAddress] }) } @@ -58,10 +79,12 @@ export const getModules = async (safeInfo: SafeInfo | void): Promise { - const [module, previousModule] = modulePair + const [previousModule, module] = modulePair const safeInstance = getGnosisSafeInstanceAt(safeAddress) return safeInstance.methods.disableModule(previousModule, module).encodeABI() diff --git a/src/routes/safe/components/Settings/Advanced/ModulesTable.tsx b/src/routes/safe/components/Settings/Advanced/ModulesTable.tsx index 37694bfc..7e67d8bf 100644 --- a/src/routes/safe/components/Settings/Advanced/ModulesTable.tsx +++ b/src/routes/safe/components/Settings/Advanced/ModulesTable.tsx @@ -50,9 +50,9 @@ const ModulesTable = ({ moduleData }: ModulesTableProps): React.ReactElement => const [viewRemoveModuleModal, setViewRemoveModuleModal] = React.useState(false) const hideRemoveModuleModal = () => setViewRemoveModuleModal(false) - const [selectedModule, setSelectedModule] = React.useState() - const triggerRemoveSelectedModule = (module: ModulePair): void => { - setSelectedModule(module) + const [selectedModulePair, setSelectedModulePair] = React.useState() + const triggerRemoveSelectedModule = (modulePair: ModulePair): void => { + setSelectedModulePair(modulePair) setViewRemoveModuleModal(true) } @@ -80,14 +80,15 @@ const ModulesTable = ({ moduleData }: ModulesTableProps): React.ReactElement => {autoColumns.map((column, index) => { const columnId = column.id const rowElement = row[columnId] + const [, moduleAddress] = rowElement return ( {columnId === MODULES_TABLE_ADDRESS_ID ? ( - - {rowElement[0]} + + {moduleAddress} ) : ( rowElement @@ -117,8 +118,8 @@ const ModulesTable = ({ moduleData }: ModulesTableProps): React.ReactElement => } - {viewRemoveModuleModal && selectedModule && ( - + {viewRemoveModuleModal && selectedModulePair && ( + )} ) diff --git a/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx b/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx index a24fc72c..53fe9075 100644 --- a/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx +++ b/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx @@ -41,21 +41,22 @@ const openIconStyle = { interface RemoveModuleModalProps { onClose: () => void - selectedModule: ModulePair + selectedModulePair: ModulePair } -const RemoveModuleModal = ({ onClose, selectedModule }: RemoveModuleModalProps): React.ReactElement => { +const RemoveModuleModal = ({ onClose, selectedModulePair }: RemoveModuleModalProps): React.ReactElement => { const classes = useStyles() const safeAddress = useSelector(safeParamAddressFromStateSelector) const dispatch = useDispatch() - const explorerInfo = getExplorerInfo(selectedModule[0]) + const [, moduleAddress] = selectedModulePair + const explorerInfo = getExplorerInfo(moduleAddress) const { url } = explorerInfo() const removeSelectedModule = async (): Promise => { try { - const txData = getDisableModuleTxData(selectedModule, safeAddress) + const txData = getDisableModuleTxData(selectedModulePair, safeAddress) dispatch( createTransaction({ @@ -67,7 +68,7 @@ const RemoveModuleModal = ({ onClose, selectedModule }: RemoveModuleModalProps): }), ) } catch (e) { - console.error(`failed to remove the module ${selectedModule}`, e.message) + console.error(`failed to remove the module ${selectedModulePair}`, e.message) } } @@ -92,16 +93,16 @@ const RemoveModuleModal = ({ onClose, selectedModule }: RemoveModuleModalProps): - + - {selectedModule[0]} + {moduleAddress} - {selectedModule[0]} + {moduleAddress}