From 4cc28df0839a3a3bd6701a152474e3985ad6f61e Mon Sep 17 00:00:00 2001 From: fernandomg Date: Tue, 24 Nov 2020 18:16:43 -0300 Subject: [PATCH 1/4] fix remove module feature - changed how the module pair is organized, now it's more natural (prev, module) tuple --- src/logic/safe/store/models/safe.ts | 7 +++- src/logic/safe/utils/modules.ts | 35 +++++++++++++++---- .../Settings/Advanced/ModulesTable.tsx | 5 +-- .../Settings/Advanced/RemoveModuleModal.tsx | 9 ++--- 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/logic/safe/store/models/safe.ts b/src/logic/safe/store/models/safe.ts index ef217e28d5..89515c76ef 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/modules.ts b/src/logic/safe/utils/modules.ts index b15e2c15a1..4a2f628b18 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 + */ +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 37694bfc87..d6eec734b0 100644 --- a/src/routes/safe/components/Settings/Advanced/ModulesTable.tsx +++ b/src/routes/safe/components/Settings/Advanced/ModulesTable.tsx @@ -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[1] return ( {columnId === MODULES_TABLE_ADDRESS_ID ? ( - - {rowElement[0]} + + {moduleAddress} ) : ( rowElement diff --git a/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx b/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx index a24fc72c98..f31006e8dd 100644 --- a/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx +++ b/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx @@ -50,7 +50,8 @@ const RemoveModuleModal = ({ onClose, selectedModule }: RemoveModuleModalProps): const safeAddress = useSelector(safeParamAddressFromStateSelector) const dispatch = useDispatch() - const explorerInfo = getExplorerInfo(selectedModule[0]) + const moduleAddress = selectedModule[1] + const explorerInfo = getExplorerInfo(moduleAddress) const { url } = explorerInfo() const removeSelectedModule = async (): Promise => { @@ -92,16 +93,16 @@ const RemoveModuleModal = ({ onClose, selectedModule }: RemoveModuleModalProps): - + - {selectedModule[0]} + {moduleAddress} - {selectedModule[0]} + {moduleAddress} From 4c8080c26f5e1f1cdc60ee1fd0c2a0f56165e99a Mon Sep 17 00:00:00 2001 From: fernandomg Date: Wed, 25 Nov 2020 12:54:45 -0300 Subject: [PATCH 2/4] rename `selectedModule` to `selectedModulePair` --- .../components/Settings/Advanced/ModulesTable.tsx | 12 ++++++------ .../Settings/Advanced/RemoveModuleModal.tsx | 10 +++++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/routes/safe/components/Settings/Advanced/ModulesTable.tsx b/src/routes/safe/components/Settings/Advanced/ModulesTable.tsx index d6eec734b0..7e67d8bf35 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,7 +80,7 @@ const ModulesTable = ({ moduleData }: ModulesTableProps): React.ReactElement => {autoColumns.map((column, index) => { const columnId = column.id const rowElement = row[columnId] - const moduleAddress = rowElement[1] + const [, moduleAddress] = rowElement return ( @@ -118,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 f31006e8dd..53fe9075da 100644 --- a/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx +++ b/src/routes/safe/components/Settings/Advanced/RemoveModuleModal.tsx @@ -41,22 +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 moduleAddress = selectedModule[1] + 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({ @@ -68,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) } } From 02ec0ce44795e1ace27a96022e9002831977b3d3 Mon Sep 17 00:00:00 2001 From: fernandomg Date: Wed, 25 Nov 2020 13:29:13 -0300 Subject: [PATCH 3/4] add tests for `buildModulesLinkedList` --- .../safe/utils/__tests__/modules.test.ts | 111 ++++++++++++++++++ src/logic/safe/utils/modules.ts | 2 +- 2 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 src/logic/safe/utils/__tests__/modules.test.ts 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 0000000000..8f813f553d --- /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 should be 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 this 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 this 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 4a2f628b18..7b93460942 100644 --- a/src/logic/safe/utils/modules.ts +++ b/src/logic/safe/utils/modules.ts @@ -28,7 +28,7 @@ type ModulesPaginated = { * @param {Array} modules * @returns null | Array */ -const buildModulesLinkedList = (modules: string[]): Array | null => { +export const buildModulesLinkedList = (modules: string[]): Array | null => { if (modules?.length) { return modules.map((moduleAddress, index, modules) => { if (index === 0) { From eed7d16cff6bba333ba44740a203fb5173030a8b Mon Sep 17 00:00:00 2001 From: fernandomg Date: Wed, 25 Nov 2020 13:52:42 -0300 Subject: [PATCH 4/4] fix typos --- src/logic/safe/utils/__tests__/modules.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/logic/safe/utils/__tests__/modules.test.ts b/src/logic/safe/utils/__tests__/modules.test.ts index 8f813f553d..ccaf4b256e 100644 --- a/src/logic/safe/utils/__tests__/modules.test.ts +++ b/src/logic/safe/utils/__tests__/modules.test.ts @@ -53,7 +53,7 @@ describe('modules -> buildModulesLinkedList', () => { // Given moduleManager.enableModule('0xc') moduleManager.enableModule('0xb') - moduleManager.enableModule('0xa') // returned list should be ordered [0xa, 0xb, 0xc] + moduleManager.enableModule('0xa') // returned list is ordered [0xa, 0xb, 0xc] const modulesPairList = buildModulesLinkedList(moduleManager.getModules()) // When @@ -69,7 +69,7 @@ describe('modules -> buildModulesLinkedList', () => { ]) }) - it(`should properly provide a list of modules pair to remove this firstly added module`, () => { + it(`should properly provide a list of modules pair to remove the firstly added module`, () => { // Given moduleManager.enableModule('0xc') moduleManager.enableModule('0xb') @@ -89,7 +89,7 @@ describe('modules -> buildModulesLinkedList', () => { ]) }) - it(`should properly provide a list of modules pair to remove this lastly added module`, () => { + it(`should properly provide a list of modules pair to remove the lastly added module`, () => { // Given moduleManager.enableModule('0xc') moduleManager.enableModule('0xb')