Fyeo did bnb 02 (#20)

1) Only allow valid flags to be added to verification methods
2) Adding an additional DID_DOC_HIDDEN flag.

---------

Reviewed-by: Martin Riedel and Robert Leonard
Co-authored-by: Robert Leonard
Co-authored-by: Martin Riedel

src/DidRegistry.sol

@@ -15,13 +15,22 @@ contract DIDRegistry is IDidRegistry, Initializable, UUPSUpgradeable, OwnableUpg
 
     // Each flag is represented by a specific bit. This enum specifies what flag corresponds to which bit.
     enum VerificationMethodFlagBitMask {
+        /// The VM can be used for encryption
         KEY_AGREEMENT, // bit 0
+        /// The VM is able to authenticate the subject
         AUTHENTICATION, // bit 1
+        /// The VM is able to proof assertions on the subject
         ASSERTION, // bit 2
+        /// The VM can be used for issuing capabilities. Required for DID Update
         CAPABILITY_INVOCATION, // bit 3
+        /// The VM can be used for delegating capabilities.
         CAPABILITY_DELEGATION, // bit 4
+        /// The subject did proof to be in possession of the private key
         OWNERSHIP_PROOF, // bit 5
-        PROTECTED // bit 6
+        /// The Verification Method is marked as protected. This means it cannot be removed
+        PROTECTED, // bit 6
+        /// The VM is hidden from the DID Document (off-chain only)
+        DID_DOC_HIDDEN //bit 7
     }
 
     struct VerificationMethod {
@@ -101,6 +110,7 @@ contract DIDRegistry is IDidRegistry, Initializable, UUPSUpgradeable, OwnableUpg
     function addVerificationMethod(address didIdentifier, VerificationMethod calldata verificationMethod) onlyNonGenerativeDid(didIdentifier) onlyAuthorizedKeys(didIdentifier) public {
 
         require(!_doesFragmentExist(didIdentifier, verificationMethod.fragment), "Fragment already exist");
+        require(_isValidFlag(verificationMethod.flags), "Attempted to add unsupported flag");
 
         // Apply a bitmask on the verificationMethodFlags
         bool hasOwnershipFlag =  _hasFlag(verificationMethod.flags, VerificationMethodFlagBitMask.OWNERSHIP_PROOF);
@@ -146,6 +156,7 @@ contract DIDRegistry is IDidRegistry, Initializable, UUPSUpgradeable, OwnableUpg
 
     function updateVerificationMethodFlags(address didIdentifier, string calldata fragment, uint16 flags) onlyNonGenerativeDid(didIdentifier) onlyAuthorizedKeys(didIdentifier) public returns(bool) {
         require(_doesFragmentExist(didIdentifier, fragment), "Fragment does not match any verification methods with this did");
+        require(_isValidFlag(flags), "Attempted to add unsupported flag");
 
         DidState storage didState = didStates[didIdentifier];
 
@@ -346,4 +357,9 @@ contract DIDRegistry is IDidRegistry, Initializable, UUPSUpgradeable, OwnableUpg
     function _hasFlag(uint16 flags, VerificationMethodFlagBitMask flag) internal pure returns(bool) {
         return flags & uint16(uint16(1) << uint16(flag)) != 0;
     }
+
+    function _isValidFlag(uint16 flags) internal pure returns(bool) {
+        // Shifts the input flag by the amount of flags avalible.
+        return uint16(flags) >> (uint16(type(VerificationMethodFlagBitMask).max) + 1) == 0;
+    }
 }

test/DidRegistryVerificationMethodTest.sol

@@ -193,9 +193,27 @@ contract DidRegistryVerificationMethodTest is DidRegistryTest {
         didRegistry.initializeDidState(user);
 
         vm.expectRevert("Cannot remove last authority verification method");
-        didRegistry.updateVerificationMethodFlags(user, 'default', uint16(0));
+        didRegistry.updateVerificationMethodFlags(user, 'default', uint16(uint16(1) << uint16(DIDRegistry.VerificationMethodFlagBitMask.DID_DOC_HIDDEN)));
     }
 
+    function test_revert_should_not_allow_adding_unknown_flag_to_verification_method() public {
+        address user = vm.addr(1);
+
+        vm.startPrank(user); // Send transaction as the user
+
+        didRegistry.initializeDidState(user);
+        DIDRegistry.DidState memory didState = didRegistry.resolveDidState(user);
+
+        DIDRegistry.VerificationMethod memory defaultVerificationMethod = didState.verificationMethods[0];
+
+        // Add none supported flag
+        uint16 newFlags = defaultVerificationMethod.flags | uint16(uint16(1) << uint16(type(DIDRegistry.VerificationMethodFlagBitMask).max) + 2);
+
+        vm.expectRevert("Attempted to add unsupported flag");
+        didRegistry.updateVerificationMethodFlags(user, defaultVerificationMethod.fragment, newFlags);
+    }
+
+
     function test_revert_only_authorized_key_can_remove_ownership_proof_flag_on_verification_method() public {
         address userOne = vm.addr(1);
         address userTwo = vm.addr(2);

ts-client/src/service/DidRegistryInternal.ts

@@ -1,10 +1,6 @@
 import { Overrides } from 'ethers';
 
-import {
-  MappedWriteOperation,
-  Options,
-  ReadOnlyOperation,
-} from '../utils';
+import { MappedWriteOperation, Options, ReadOnlyOperation } from '../utils';
 import { omit } from 'ramda';
 import { DIDRegistry } from '../contracts/typechain-types';
 import { DidIdentifier } from './DidIdentifier';

ts-client/src/utils/doc.ts

@@ -23,6 +23,7 @@ export enum BitwiseVerificationMethodFlag {
   CapabilityDelegation = 1 << 4,
   OwnershipProof = 1 << 5,
   Protected = 1 << 6,
+  DidDocHidden = 1 << 7,
 }
 
 export enum VerificationMethodType {
@@ -56,7 +57,7 @@ export type RawVerificationMethod = {
   keyData: string;
 };
 
-export type ChainEnviroment = 'mainnet' | 'testnet' | 'localnet'
+export type ChainEnviroment = 'mainnet' | 'testnet' | 'localnet';
 
 export type RawService = {
   fragment: string;
@@ -78,10 +79,9 @@ export const mapVerificationMethodsToDidComponents = (
   };
 
   for (const method of methods) {
-    // skip hidden methods (TODO: do we want to suppor this again?)
-    // if (method.flags.has(BitwiseVerificationMethodFlag.DidDocHidden)) {
-    //   continue;
-    // }
+    if (method.flags.has(BitwiseVerificationMethodFlag.DidDocHidden)) {
+      continue;
+    }
     if (method.flags.has(BitwiseVerificationMethodFlag.Authentication)) {
       didComponents.authentication.push(
         `${identifier.toString()}#${method.fragment}`

ts-client/test/integration/DidRegistry.test.ts

@@ -53,15 +53,23 @@ describe('Native TS Client Integration Test', () => {
     const { address } = await deployDidRegistryContractInstance(deployerWallet);
 
     // Connect didWallet and otherDidWallet to didRegistryContract
-    didRegistry = new DidRegistry(didWallet, address, {chainEnvironment: 'localnet'});
-    otherDidRegistry = new DidRegistry(otherDidWallet, address, {chainEnvironment: 'localnet'});
+    didRegistry = new DidRegistry(didWallet, address, {
+      chainEnvironment: 'localnet',
+    });
+    otherDidRegistry = new DidRegistry(otherDidWallet, address, {
+      chainEnvironment: 'localnet',
+    });
     otherDidRegistry.setDidIdentifier(didWallet.address); // otherDidWallet is message sender, but DID is still from didWallet
-    unauthorizedDidRegistry = new DidRegistry(unauthorizedWallet, address, {chainEnvironment: 'localnet'});
+    unauthorizedDidRegistry = new DidRegistry(unauthorizedWallet, address, {
+      chainEnvironment: 'localnet',
+    });
     unauthorizedDidRegistry.setDidIdentifier(didWallet.address); // unauthorizedDidWallet is message sender, but DID is still from didWallet
   });
 
   it('should generate DIDs with the correct prefix', () => {
-    expect(didRegistry.getDid().toString()).to.be.a('string').and.satisfy((did: string) => did.startsWith('did:bnb:localnet:'));
+    expect(didRegistry.getDid().toString())
+      .to.be.a('string')
+      .and.satisfy((did: string) => did.startsWith('did:bnb:localnet:'));
   });
 
   it('should resolve an initial DID State of the didWallet', async () => {
@@ -233,6 +241,29 @@ describe('Native TS Client Integration Test', () => {
     );
   });
 
+  it('should hide verification methods in the did document if the related flag is set', async () => {
+    const currentVerificationMethods = await otherDidRegistry
+      .resolve()
+      .then((doc) => doc.verificationMethod);
+
+    const tx = await otherDidRegistry
+      .setVerificationMethodFlags(SECONDARY_KEY_ID, [
+        BitwiseVerificationMethodFlag.DidDocHidden,
+        BitwiseVerificationMethodFlag.CapabilityInvocation,
+      ])
+      .then((tx) => tx.wait());
+    expect(tx.status).to.equal(1);
+
+    const doc = await otherDidRegistry.resolve();
+    expect(doc.verificationMethod).to.have.lengthOf(
+      (currentVerificationMethods?.length || Number.NaN) - 1
+    );
+    // make sure the hidden verification method is not in the did document
+    expect(
+      doc.verificationMethod?.find((vm) => vm.id.endsWith(SECONDARY_KEY_ID))
+    ).to.be.undefined;
+  });
+
   it('should not able set Ownership proof for a different key', async () => {
     return expect(
       didRegistry.setVerificationMethodFlags(SECONDARY_KEY_ID, [