(Fix) remove module feature (#1646)

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

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'],
+    ])
+  })
+})

src/logic/safe/utils/modules.ts

@@ -9,11 +9,32 @@ type ModulesPaginated = {
   next: string
 }
 
-const buildModulesLinkedList = (modules: string[], nextModule: string = SENTINEL_ADDRESS): Array<ModulePair> | 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<string>} modules
+ * @returns null | Array<ModulePair>
+ */
+export const buildModulesLinkedList = (modules: string[]): Array<ModulePair> | null => {
   if (modules?.length) {
     return modules.map((moduleAddress, index, modules) => {
-      const prevModule = modules[index + 1]
+      if (index === 0) {
-      return [moduleAddress, prevModule !== undefined ? prevModule : nextModule]
+        return [SENTINEL_ADDRESS, moduleAddress]
+      }
+      return [modules[index - 1], moduleAddress]
     })
   }
 
@@ -58,10 +79,12 @@ export const getModules = async (safeInfo: SafeInfo | void): Promise<Array<Modul
       // as we're not sure if there are more than 10 modules enabled for the current Safe
       const safeInstance = getGnosisSafeInstanceAt(safeInfo.address)
 
-      // TODO: 100 is an arbitrary large number, to avoid the need for pagination. But pagination must be properly handled
+      // TODO: 100 is an arbitrary large number, to avoid the need for pagination.
+      //  But pagination must be properly handled
+      //  if `modules.next !== SENTINEL_ADDRESS`, then we have more modules to retrieve
       const modules: ModulesPaginated = await safeInstance.methods.getModulesPaginated(SENTINEL_ADDRESS, 100).call()
 
-      return buildModulesLinkedList(modules.array, modules.next)
+      return buildModulesLinkedList(modules.array)
     } catch (e) {
       console.error('Failed to retrieve Safe modules', e)
     }
@@ -69,7 +92,7 @@ export const getModules = async (safeInfo: SafeInfo | void): Promise<Array<Modul
 }
 
 export const getDisableModuleTxData = (modulePair: ModulePair, safeAddress: string): string => {
-  const [module, previousModule] = modulePair
+  const [previousModule, module] = modulePair
   const safeInstance = getGnosisSafeInstanceAt(safeAddress)
 
   return safeInstance.methods.disableModule(previousModule, module).encodeABI()

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<ModulePair>()
+  const [selectedModulePair, setSelectedModulePair] = React.useState<ModulePair>()
-  const triggerRemoveSelectedModule = (module: ModulePair): void => {
+  const triggerRemoveSelectedModule = (modulePair: ModulePair): void => {
-    setSelectedModule(module)
+    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 (
                     <React.Fragment key={`${columnId}-${index}`}>
                       <TableCell align={column.align} component="td" key={columnId}>
                         {columnId === MODULES_TABLE_ADDRESS_ID ? (
                           <Block justify="left">
-                            <Identicon address={rowElement[0]} diameter={32} />
+                            <Identicon address={moduleAddress} diameter={32} />
-                            <AddressText size="lg">{rowElement[0]}</AddressText>
+                            <AddressText size="lg">{moduleAddress}</AddressText>
                           </Block>
                         ) : (
                           rowElement
@@ -117,8 +118,8 @@ const ModulesTable = ({ moduleData }: ModulesTableProps): React.ReactElement =>
           }
         </Table>
       </TableContainer>
-      {viewRemoveModuleModal && selectedModule && (
+      {viewRemoveModuleModal && selectedModulePair && (
-        <RemoveModuleModal onClose={hideRemoveModuleModal} selectedModule={selectedModule} />
+        <RemoveModuleModal onClose={hideRemoveModuleModal} selectedModulePair={selectedModulePair} />
       )}
     </>
   )

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<void> => {
     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):
         <Block className={classes.modalContainer}>
           <Row className={classes.modalOwner}>
             <Col align="center" xs={1}>
-              <Identicon address={selectedModule[0]} diameter={32} />
+              <Identicon address={moduleAddress} diameter={32} />
             </Col>
             <Col xs={11}>
               <Block className={cn(classes.modalName, classes.modalUserName)}>
                 <Paragraph noMargin size="lg" weight="bolder">
-                  {selectedModule[0]}
+                  {moduleAddress}
                 </Paragraph>
                 <Block className={classes.modalUser} justify="center">
                   <Paragraph color="disabled" noMargin size="md">
-                    {selectedModule[0]}
+                    {moduleAddress}
                   </Paragraph>
                   <Link className={classes.modalOpen} target="_blank" to={url}>
                     <OpenInNew style={openIconStyle} />