fix(domains.service): remove addressOf from owner param

Previous `parseDid` function returned address if it was just an address. However, I think that was confusing behaviour It's better to validate that param is an address
energywebfoundation · Dec 7, 2021 · 2458507 · 2458507
1 parent 4355ebb
commit 2458507
Show file tree

Hide file tree

Showing 15 changed files with 186 additions and 9 deletions.
diff --git a/...api/classes/errors_ENSOwnerNotValidAddressError.ENSOwnerNotValidAddressError.md b/...api/classes/errors_ENSOwnerNotValidAddressError.ENSOwnerNotValidAddressError.md
@@ -0,0 +1,31 @@
+# Class: ENSOwnerNotValidAddressError
+
+[errors/ENSOwnerNotValidAddressError](../modules/errors_ENSOwnerNotValidAddressError.md).ENSOwnerNotValidAddressError
+
+## Hierarchy
+
+- `Error`
+
+  ↳ **`ENSOwnerNotValidAddressError`**
+
+## Table of contents
+
+### Constructors
+
+- [constructor](errors_ENSOwnerNotValidAddressError.ENSOwnerNotValidAddressError.md#constructor)
+
+## Constructors
+
+### constructor
+
+• **new ENSOwnerNotValidAddressError**(`providedOwner`)
+
+#### Parameters
+
+| Name | Type |
+| :------ | :------ |
+| `providedOwner` | `string` |
+
+#### Overrides
+
+Error.constructor
diff --git a/docs/api/enums/errors_ErrorMessages.ERROR_MESSAGES.md b/docs/api/enums/errors_ErrorMessages.ERROR_MESSAGES.md
@@ -9,6 +9,7 @@
 - [APP\_WITH\_ROLES](errors_ErrorMessages.ERROR_MESSAGES.md#app_with_roles)
 - [CAN\_NOT\_UPDATE\_NOT\_CONTROLLED\_DOCUMENT](errors_ErrorMessages.ERROR_MESSAGES.md#can_not_update_not_controlled_document)
 - [CLAIM\_WAS\_NOT\_ISSUED](errors_ErrorMessages.ERROR_MESSAGES.md#claim_was_not_issued)
+- [ENS\_OWNER\_NOT\_VALID\_ADDRESS](errors_ErrorMessages.ERROR_MESSAGES.md#ens_owner_not_valid_address)
 - [ENS\_TYPE\_NOT\_SUPPORTED](errors_ErrorMessages.ERROR_MESSAGES.md#ens_type_not_supported)
 - [ERROR\_IN\_AZURE\_PROVIDER](errors_ErrorMessages.ERROR_MESSAGES.md#error_in_azure_provider)
 - [INSUFFICIENT\_BALANCE](errors_ErrorMessages.ERROR_MESSAGES.md#insufficient_balance)
@@ -47,6 +48,12 @@ ___
 
 ___
 
+### ENS\_OWNER\_NOT\_VALID\_ADDRESS
+
+• **ENS\_OWNER\_NOT\_VALID\_ADDRESS** = `"Provided owner is not a valid address. Owner of ENS domain must be an address"`
+
+___
+
 ### ENS\_TYPE\_NOT\_SUPPORTED
 
 • **ENS\_TYPE\_NOT\_SUPPORTED** = `"ENS type not supported"`

diff --git a/docs/api/modules.md b/docs/api/modules.md
@@ -11,6 +11,7 @@
 - [errors](modules/errors.md)
 - [errors/ChangeOwnershipNotPossibleError](modules/errors_ChangeOwnershipNotPossibleError.md)
 - [errors/DeletingNamespaceNotPossibleError](modules/errors_DeletingNamespaceNotPossibleError.md)
+- [errors/ENSOwnerNotValidAddressError](modules/errors_ENSOwnerNotValidAddressError.md)
 - [errors/ENSTypeNotSupportedError](modules/errors_ENSTypeNotSupportedError.md)
 - [errors/ErrorMessages](modules/errors_ErrorMessages.md)
 - [errors/MethodNotAvailableInNodeError](modules/errors_MethodNotAvailableInNodeError.md)
@@ -49,6 +50,7 @@
 - [modules/staking/staking.service](modules/modules_staking_staking_service.md)
 - [utils](modules/utils.md)
 - [utils/ENS\_hash](modules/utils_ENS_hash.md)
+- [utils/address](modules/utils_address.md)
 - [utils/change\_resolver](modules/utils_change_resolver.md)
 - [utils/constants](modules/utils_constants.md)
 - [utils/detectEnvironment](modules/utils_detectEnvironment.md)

diff --git a/docs/api/modules/errors.md b/docs/api/modules/errors.md
@@ -6,6 +6,7 @@
 
 - [ChangeOwnershipNotPossibleError](errors.md#changeownershipnotpossibleerror)
 - [DeletingNamespaceNotPossibleError](errors.md#deletingnamespacenotpossibleerror)
+- [ENSOwnerNotValidAddressError](errors.md#ensownernotvalidaddresserror)
 - [ENSTypeNotSupportedError](errors.md#enstypenotsupportederror)
 - [ERROR\_MESSAGES](errors.md#error_messages)
 - [MethodNotAvailableInNodeEnvError](errors.md#methodnotavailableinnodeenverror)
@@ -24,6 +25,12 @@ Re-exports: [DeletingNamespaceNotPossibleError](../classes/errors_DeletingNamesp
 
 ___
 
+### ENSOwnerNotValidAddressError
+
+Re-exports: [ENSOwnerNotValidAddressError](../classes/errors_ENSOwnerNotValidAddressError.ENSOwnerNotValidAddressError.md)
+
+___
+
 ### ENSTypeNotSupportedError
 
 Re-exports: [ENSTypeNotSupportedError](../classes/errors_ENSTypeNotSupportedError.ENSTypeNotSupportedError.md)

diff --git a/docs/api/modules/errors_ENSOwnerNotValidAddressError.md b/docs/api/modules/errors_ENSOwnerNotValidAddressError.md
@@ -0,0 +1,7 @@
+# Module: errors/ENSOwnerNotValidAddressError
+
+## Table of contents
+
+### Classes
+
+- [ENSOwnerNotValidAddressError](../classes/errors_ENSOwnerNotValidAddressError.ENSOwnerNotValidAddressError.md)
diff --git a/docs/api/modules/index.md b/docs/api/modules/index.md
@@ -25,6 +25,7 @@
 - [DeletingNamespaceNotPossibleError](index.md#deletingnamespacenotpossibleerror)
 - [DidRegistry](index.md#didregistry)
 - [DomainsService](index.md#domainsservice)
+- [ENSOwnerNotValidAddressError](index.md#ensownernotvalidaddresserror)
 - [ENSTypeNotSupportedError](index.md#enstypenotsupportederror)
 - [ERROR\_MESSAGES](index.md#error_messages)
 - [EkcSigner](index.md#ekcsigner)
@@ -228,6 +229,12 @@ Re-exports: [DomainsService](../classes/modules_domains_domains_service.DomainsS
 
 ___
 
+### ENSOwnerNotValidAddressError
+
+Re-exports: [ENSOwnerNotValidAddressError](../classes/errors_ENSOwnerNotValidAddressError.ENSOwnerNotValidAddressError.md)
+
+___
+
 ### ENSTypeNotSupportedError
 
 Re-exports: [ENSTypeNotSupportedError](../classes/errors_ENSTypeNotSupportedError.ENSTypeNotSupportedError.md)

diff --git a/docs/api/modules/utils_address.md b/docs/api/modules/utils_address.md
@@ -0,0 +1,28 @@
+# Module: utils/address
+
+## Table of contents
+
+### Functions
+
+- [validateAddress](utils_address.md#validateaddress)
+
+## Functions
+
+### validateAddress
+
+▸ **validateAddress**(`address`): `void`
+
+Validate that address is valid ethereum address.
+Expect that error is thrown if not
+Uses ethers function but encapsulates to be able to swap in the future:
+https://docs.ethers.io/v5/api/utils/address/#utils-getAddress
+
+#### Parameters
+
+| Name | Type | Description |
+| :------ | :------ | :------ |
+| `address` | `string` | address to verify |
+
+#### Returns
+
+`void`
diff --git a/e2e/domains.service.e2e.ts b/e2e/domains.service.e2e.ts
@@ -6,6 +6,7 @@ import {
     initWithPrivateKeySigner,
     MessagingService,
     NamespaceType,
+    ENSOwnerNotValidAddressError,
     RegistrationTypes,
     SignerService,
     StakingService,
@@ -129,4 +130,53 @@ describe("Domains service", () => {
             expect(actualTypes).toEqual(expectedTypes);
         });
     });
+
+    /**
+     * EWC/Volta ENS domains can currently only be owned by EWC/Volta (i.e. EVM) addresses.
+     * The ENS contract does not support DID ownership.
+     * See the following for background discussion
+     * - https://energyweb.atlassian.net/browse/SWTCH-790?focusedCommentId=15216
+     * - https://energyweb.atlassian.net/browse/SWTCH-1050
+     * TODO: These tests should probably be unit tests, not e2e,
+     */
+    describe("owner param validation", () => {
+        test("getENSTypesByOwner should throw if owner is not an address", () => {
+            expect(() =>
+                domainsService.getENSTypesByOwner({
+                    type: NamespaceType.Organization,
+                    owner: `did:ethr:volta:${rootOwner.address}`,
+                }),
+            ).toThrow(ENSOwnerNotValidAddressError);
+        });
+
+        test("changeRoleOwnership should throw if newOwner is not an address", async () => {
+            const did = `did:ethr:volta:${rootOwner.address}`;
+            await expect(
+                domainsService.changeRoleOwnership({
+                    namespace: "somerole.roles.energyweb.iam.ewc",
+                    newOwner: did,
+                }),
+            ).rejects.toEqual(new ENSOwnerNotValidAddressError(did));
+        });
+
+        test("changeAppOwnership should throw if newOwner is not an address", async () => {
+            const did = `did:ethr:volta:${rootOwner.address}`;
+            await expect(
+                domainsService.changeAppOwnership({
+                    namespace: "someapp.app.energyweb.iam.ewc",
+                    newOwner: did,
+                }),
+            ).rejects.toEqual(new ENSOwnerNotValidAddressError(did));
+        });
+
+        test("changeOrgOwnership should throw if newOwner is not an address", async () => {
+            const did = `did:ethr:volta:${rootOwner.address}`;
+            await expect(
+                domainsService.changeOrgOwnership({
+                    namespace: "energyweb.iam.ewc",
+                    newOwner: did,
+                }),
+            ).rejects.toEqual(new ENSOwnerNotValidAddressError(did));
+        });
+    });
 });
diff --git a/e2e/staking.pool.e2e.ts b/e2e/staking.pool.e2e.ts
@@ -112,7 +112,7 @@ describe("StakingPool tests", () => {
         mockGetApplicationsByOrgNamespace.mockReturnValueOnce([]);
         await domainsService.changeOrgOwnership({
             namespace: `${orgName}.${root}`,
-            newOwner: orgOwnerDid,
+            newOwner: orgOwner.address,
         });
 
         const registrationTypes = [RegistrationTypes.OnChain];

diff --git a/e2e/staking.service.e2e.ts b/e2e/staking.service.e2e.ts
@@ -105,7 +105,7 @@ describe("Staking service tests", () => {
         mockGetApplicationsByOrgNamespace.mockReturnValueOnce([]);
         await domainsService.changeOrgOwnership({
             namespace: `${orgName}.${root}`,
-            newOwner: orgOwnerDid,
+            newOwner: orgOwner.address,
         });
 
         const registrationTypes = [RegistrationTypes.OnChain];
@@ -179,7 +179,7 @@ describe("Staking service tests", () => {
         });
         await domainsService.changeOrgOwnership({
             namespace: `${orgName2}.${root}`,
-            newOwner: orgOwnerDid,
+            newOwner: orgOwner.address,
         });
 
         await signerService.connect(orgOwner, ProviderType.PrivateKey);

diff --git a/src/errors/ENSOwnerNotValidAddressError.ts b/src/errors/ENSOwnerNotValidAddressError.ts
@@ -0,0 +1,7 @@
+import { ERROR_MESSAGES } from "./ErrorMessages";
+
+export class ENSOwnerNotValidAddressError extends Error {
+    constructor(providedOwner: string) {
+        super(`Provided owner param: ${providedOwner}. ${ERROR_MESSAGES.ENS_OWNER_NOT_VALID_ADDRESS}`);
+    }
+}
diff --git a/src/errors/ErrorMessages.ts b/src/errors/ErrorMessages.ts
@@ -19,4 +19,5 @@ export enum ERROR_MESSAGES {
     ERROR_IN_AZURE_PROVIDER = "Error in Azure Provider",
     JWT_ALGORITHM_NOT_SUPPORTED = "Jwt algorithm not supported",
     CLAIM_WAS_NOT_ISSUED = "Claim was not issued",
+    ENS_OWNER_NOT_VALID_ADDRESS = "Provided owner is not a valid address. Owner of ENS domain must be an address",
 }
diff --git a/src/errors/index.ts b/src/errors/index.ts
@@ -2,12 +2,14 @@ import { ENSTypeNotSupportedError } from "./ENSTypeNotSupportedError";
 import { MethodNotAvailableInNodeEnvError } from "./MethodNotAvailableInNodeError";
 import { ChangeOwnershipNotPossibleError } from "./ChangeOwnershipNotPossibleError";
 import { DeletingNamespaceNotPossibleError } from "./DeletingNamespaceNotPossibleError";
+import { ENSOwnerNotValidAddressError } from "./ENSOwnerNotValidAddressError";
 import { ERROR_MESSAGES } from "./ErrorMessages";
 
 export {
     ENSTypeNotSupportedError,
     MethodNotAvailableInNodeEnvError,
     ChangeOwnershipNotPossibleError,
     DeletingNamespaceNotPossibleError,
+    ENSOwnerNotValidAddressError,
     ERROR_MESSAGES,
 };
diff --git a/src/modules/domains/domains.service.ts b/src/modules/domains/domains.service.ts
@@ -9,14 +9,14 @@ import {
     ResolverContractType,
     DomainHierarchy,
 } from "@energyweb/iam-contracts";
-import { addressOf } from "@ew-did-registry/did-ethr-resolver";
 import { ENSRegistry } from "../../../ethers/ENSRegistry";
 import { ENSRegistry__factory } from "../../../ethers/factories/ENSRegistry__factory";
 import { chainConfigs } from "../../config/chain.config";
 import {
     ChangeOwnershipNotPossibleError,
     DeletingNamespaceNotPossibleError,
     ENSTypeNotSupportedError,
+    ENSOwnerNotValidAddressError,
     ERROR_MESSAGES,
 } from "../../errors";
 import { emptyAddress } from "../../utils/constants";
@@ -26,6 +26,7 @@ import { RegistrationTypes } from "../claims/claims.types";
 import { SignerService } from "../signer/signer.service";
 import { NamespaceType, IOrganization } from "./domains.types";
 import { SearchType } from "../cacheClient/cacheClient.types";
+import { validateAddress } from "../../utils/address";
 
 const { namehash } = utils;
 
@@ -305,8 +306,8 @@ export class DomainsService {
      * changeOrgOwnership
      *
      * @description change owner ship of org subdomain and all org owned roles subdomains
+     * @param params.newOwner address of new owner
      * @returns return array of steps needed to change ownership
-     *
      */
     async changeOrgOwnership({
         namespace,
@@ -317,7 +318,7 @@ export class DomainsService {
         newOwner: string;
         returnSteps?: boolean;
     }) {
-        newOwner = addressOf(newOwner);
+        DomainsService.validateOwnerAddress(newOwner);
         const orgNamespaces = [
             `${NamespaceType.Role}.${namespace}`,
             `${NamespaceType.Application}.${namespace}`,
@@ -373,6 +374,7 @@ export class DomainsService {
      * changeAppOwnership
      *
      * @description change owner ship of app subdomain and all app owned subdomains
+     * @param params.newOwner address of new owner
      * @returns return array of steps needed to change ownership
      *
      */
@@ -385,7 +387,7 @@ export class DomainsService {
         newOwner: string;
         returnSteps?: boolean;
     }) {
-        newOwner = addressOf(newOwner);
+        DomainsService.validateOwnerAddress(newOwner);
         const appNamespaces = [`${NamespaceType.Role}.${namespace}`, namespace];
 
         const { alreadyFinished, changeOwnerNamespaces, notOwnedNamespaces } = await this.validateChangeOwnership({
@@ -429,10 +431,11 @@ export class DomainsService {
      * changeRoleOwnership
      *
      * @description change ownership of role subdomain
+     * @param params.newOwner address of new owner
      *
      */
     async changeRoleOwnership({ namespace, newOwner }: { namespace: string; newOwner: string }) {
-        newOwner = addressOf(newOwner);
+        DomainsService.validateOwnerAddress(newOwner);
         const notOwnedNamespaces = await this.validateOwnership({
             namespace,
             type: NamespaceType.Role,
@@ -641,6 +644,7 @@ export class DomainsService {
 
     /**
      * getENSTypesByOwner
+     * @param params.owner address of owner
      */
     getENSTypesByOwner({
         type,
@@ -651,7 +655,7 @@ export class DomainsService {
         owner: string;
         withRelations?: boolean;
     }) {
-        owner = addressOf(owner);
+        DomainsService.validateOwnerAddress(owner);
         if (type === NamespaceType.Organization) {
             return this._cacheClient.getOrganizationsByOwner(owner, withRelations);
         }
@@ -1014,4 +1018,16 @@ export class DomainsService {
             }),
         );
     }
+
+    /**
+     * Checks that a provided owner/newOwner address is a valid ethereum address
+     * @param owner owner address to validate
+     */
+    private static validateOwnerAddress(owner: string) {
+        try {
+            validateAddress(owner);
+        } catch {
+            throw new ENSOwnerNotValidAddressError(owner);
+        }
+    }
 }
diff --git a/src/utils/address.ts b/src/utils/address.ts
@@ -0,0 +1,12 @@
+import { utils } from "ethers";
+
+/**
+ * Validate that address is valid ethereum address.
+ * Expect that error is thrown if not
+ * Uses ethers function but encapsulates to be able to swap in the future:
+ * https://docs.ethers.io/v5/api/utils/address/#utils-getAddress
+ * @param address address to verify
+ */
+export function validateAddress(address: string) {
+    utils.getAddress(address);
+}