Skip to content

Commit

Permalink
address pull request feedback on issue 56
Browse files Browse the repository at this point in the history
  • Loading branch information
@jac18281828
jac18281828 committed Oct 10, 2023
1 parent 578a628 commit 96dd824
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 113 deletions.
4 changes: 2 additions & 2 deletions packages/did-eth-registry/test/EthereumDIDRegistry.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ contract EthereumDIDRegistryTest is Test {
vm.prank(owner);
registry.changeOwner(owner, newOwner);
vm.prank(newOwner);
registry.changeOwner(newOwner, newOwner2);
assertEq(registry.identityOwner(owner), newOwner, "should change owner");
registry.changeOwner(owner, newOwner2);
assertEq(registry.identityOwner(owner), newOwner2, "should change owner twice");
}

function testChangeOwnerOriginalAddressIsBadActor() public {
Expand Down
81 changes: 5 additions & 76 deletions packages/did-eth-registry/test/EthereumDIDRegistryAttribute.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,7 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.changeOwner(SIGNER, SIGNER2);
vm.expectEmit();
emit DIDAttributeChanged(SIGNER, "key", "value", 1 weeks + 1, block.number);
(uint8 v, bytes32 r, bytes32 s) = signAttribute(
SIGNER,
SIGNER2,
address(registry),
"key",
"value",
PRIVATE_KEY2
);
(uint8 v, bytes32 r, bytes32 s) = signAttribute(SIGNER, address(registry), "key", "value", PRIVATE_KEY2);
registry.setAttributeSigned(SIGNER, v, r, s, "key", "value", 1 weeks);
}

Expand Down Expand Up @@ -216,7 +209,7 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.setAttribute(SIGNER, "key", "value", 1 weeks);
vm.prank(SIGNER);
registry.changeOwner(SIGNER, SIGNER2);
(uint8 v, bytes32 r, bytes32 s) = signRevoke(SIGNER, SIGNER2, address(registry), "key", "value", PRIVATE_KEY2);
(uint8 v, bytes32 r, bytes32 s) = signRevoke(SIGNER, address(registry), "key", "value", PRIVATE_KEY2);
vm.expectEmit();
emit DIDAttributeChanged(SIGNER, "key", "value", 0, block.number);
registry.revokeAttributeSigned(SIGNER, v, r, s, "key", "value");
Expand All @@ -225,7 +218,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
/**
* @dev Sign data with a private key
* @param _identity Identity address
* @param _delegate Delegate address
* @param _registry DID Registry address
* @param _privateKey Private key
* @param _message Message to sign
Expand All @@ -235,7 +227,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
*/
function signData(
address _identity,
address _delegate,
address _registry,
bytes memory _privateKey,
bytes memory _message
Expand All @@ -248,7 +239,7 @@ contract EthereumDIDRegistryDelegateTest is Test {
bytes32 s
)
{
address idOwner = registry.identityOwner(_delegate);
address idOwner = registry.identityOwner(_identity);
uint256 ownerNonce = registry.nonce(idOwner);
return VmDigest.signData(vm, _identity, _registry, _privateKey, _message, ownerNonce);
}
Expand Down Expand Up @@ -280,68 +271,7 @@ contract EthereumDIDRegistryDelegateTest is Test {
)
{
bytes memory message = abi.encodePacked(bytes("setAttribute"), _key, _value, uint256(1 weeks));
return signData(_identity, _identity, _registry, _privateKey, message);
}

/**
* @dev Sign attribute with a private key
* @param _identity Identity address
* @param _registry DID Registry address
* @param _key Attribute key
* @param _value Attribute value
* @param _privateKey Private key
* @return v Signature v
* @return r Signature r
* @return s Signature s
*/
function signAttribute(
address _identity,
address _delegate,
address _registry,
bytes32 _key,
bytes memory _value,
bytes memory _privateKey
)
internal
view
returns (
uint8 v,
bytes32 r,
bytes32 s
)
{
bytes memory message = abi.encodePacked(bytes("setAttribute"), _key, _value, uint256(1 weeks));
return signData(_identity, _delegate, _registry, _privateKey, message);
}

/**
* @dev Sign revoke with a private key
* @param _identity Identity address
* @param _registry DID Registry address
* @param _key Attribute key
* @param _value Attribute value
* @param _privateKey Private key
* @return v Signature v
* @return r Signature r
* @return s Signature s
*/
function signRevoke(
address _identity,
address _registry,
bytes32 _key,
bytes memory _value,
bytes memory _privateKey
)
internal
view
returns (
uint8 v,
bytes32 r,
bytes32 s
)
{
bytes memory message = abi.encodePacked(bytes("revokeAttribute"), _key, _value);
return signData(_identity, _identity, _registry, _privateKey, message);
return signData(_identity, _registry, _privateKey, message);
}

/**
Expand All @@ -357,7 +287,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
*/
function signRevoke(
address _identity,
address _delegate,
address _registry,
bytes32 _key,
bytes memory _value,
Expand All @@ -372,6 +301,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
)
{
bytes memory message = abi.encodePacked(bytes("revokeAttribute"), _key, _value);
return signData(_identity, _delegate, _registry, _privateKey, message);
return signData(_identity, _registry, _privateKey, message);
}
}
37 changes: 5 additions & 32 deletions packages/did-eth-registry/test/EthereumDIDRegistryDelegate.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.addDelegate(SIGNER, "attestor", SIGNER2, 1 days);
}

function testAddDelegateOwnerIdentityChanged() public {
function testAddDelegateDoesNotChangeOwner() public {
assertEq(registry.owners(SIGNER), address(0x0));
vm.prank(SIGNER);
registry.addDelegate(SIGNER, "attestor", SIGNER2, 1 days);
Expand All @@ -91,17 +91,8 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.addDelegate(SIGNER, "attestor", SIGNER2, 1 days);
}

function testAddDelegateNoTakeBack() public {
vm.prank(SIGNER);
registry.changeOwner(SIGNER, SIGNER2);
vm.prank(SIGNER);
vm.expectRevert("bad_actor");
registry.addDelegate(SIGNER, "attestor", SIGNER3, 1 days);
}

function testAddDelegateSigned() public {
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER3);
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
assertTrue(registry.validDelegate(SIGNER, "attestor", SIGNER2));
}
Expand All @@ -111,14 +102,12 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.changeOwner(SIGNER, SIGNER3);
assertEq(registry.owners(SIGNER), SIGNER3);
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER3);
vm.expectRevert("bad_signature");
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
}

function testAddDelegateSignedExpires() public {
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER3);
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
assertTrue(registry.validDelegate(SIGNER, "attestor", SIGNER2));
vm.warp(1 days + 1);
Expand All @@ -127,28 +116,24 @@ contract EthereumDIDRegistryDelegateTest is Test {

function testAddDelegateSignedBadSignatureKey() public {
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER2);
vm.expectRevert("bad_signature");
registry.addDelegateSigned(SIGNER2, v, r, s, "attestor", SIGNER2, 1 days);
}

function testAddDelegateSignedWrongSignature() public {
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER2, address(registry), PRIVATE_KEY2);
vm.expectRevert("bad_signature");
vm.prank(SIGNER3);
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
}

function testAddDelegateSignedWrongSignatureData() public {
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER, address(registry), PRIVATE_KEY);
vm.expectRevert("bad_signature");
vm.prank(SIGNER3);
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
}

function testAddDelegateSignedChangedBlockNumber() public {
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER);
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
uint256 transactionBlock = registry.changed(SIGNER);
assertEq(block.number, transactionBlock, "should record block number");
Expand All @@ -158,13 +143,11 @@ contract EthereumDIDRegistryDelegateTest is Test {
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.expectEmit();
emit DIDDelegateChanged(SIGNER, "attestor", SIGNER2, 1 days + 1, 0);
vm.prank(SIGNER);
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
}

function testAddDelegateSignedExpectNonce() public {
(uint8 v, bytes32 r, bytes32 s) = signDelegate(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER);
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
uint256 nonce = registry.nonce(SIGNER);
assertEq(nonce, 1, "should increment nonce");
Expand All @@ -182,7 +165,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
message,
ownerNonce
);
vm.prank(SIGNER);
vm.expectRevert("bad_signature");
registry.addDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2, 1 days);
}
Expand Down Expand Up @@ -254,7 +236,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.addDelegate(SIGNER, "attestor", SIGNER2, 1 days);
assertTrue(registry.validDelegate(SIGNER, "attestor", SIGNER2));
(uint8 v, bytes32 r, bytes32 s) = signRevoke(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER);
registry.revokeDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2);
assertFalse(registry.validDelegate(SIGNER, "attestor", SIGNER2));
}
Expand All @@ -266,7 +247,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
assertEq(block.number, registry.changed(SIGNER), "add should record block number");
vm.roll(block.number + 100);
(uint8 v, bytes32 r, bytes32 s) = signRevoke(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER);
registry.revokeDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2);
assertEq(block.number, registry.changed(SIGNER), "revoke should record block number");
assertNotEq(block.number, startBlock, "block number must be different across calls");
Expand All @@ -280,7 +260,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
vm.expectEmit();
emit DIDDelegateChanged(SIGNER, "attestor", SIGNER2, startTime, startBlock);
(uint8 v, bytes32 r, bytes32 s) = signRevoke(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.prank(SIGNER3);
registry.revokeDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2);
}

Expand All @@ -289,10 +268,8 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.addDelegate(SIGNER, "attestor", SIGNER2, 1 days);
(uint8 v, bytes32 r, bytes32 s) = signRevoke(SIGNER, SIGNER2, address(registry), PRIVATE_KEY);
vm.expectRevert("bad_signature");
vm.prank(SIGNER3);
registry.revokeDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER3);
vm.expectRevert("bad_signature");
vm.prank(SIGNER3);
registry.revokeDelegateSigned(SIGNER3, v, r, s, "attestor", SIGNER);
}

Expand All @@ -301,7 +278,7 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.addDelegate(SIGNER, "attestor", SIGNER2, 1 days);
(uint8 v, bytes32 r, bytes32 s) = signRevoke(SIGNER, SIGNER2, address(registry), PRIVATE_KEY2);
vm.expectRevert("bad_signature");
vm.prank(SIGNER);
vm.prank(SIGNER3);
registry.revokeDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2);
}

Expand All @@ -310,7 +287,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
registry.addDelegate(SIGNER, "attestor", SIGNER2, 1 days);
(uint8 v, bytes32 r, bytes32 s) = signRevoke(SIGNER, SIGNER, address(registry), PRIVATE_KEY);
vm.expectRevert("bad_signature");
vm.prank(SIGNER);
registry.revokeDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2);
}

Expand All @@ -328,15 +304,13 @@ contract EthereumDIDRegistryDelegateTest is Test {
message,
ownerNonce
);
vm.prank(SIGNER);
vm.expectRevert("bad_signature");
registry.revokeDelegateSigned(SIGNER, v, r, s, "attestor", SIGNER2);
}

/**
* @dev Sign data with private key
* @param _identity Identity address
* @param _delegate Delegate address
* @param _registry DID Registry address
* @param _privateKey Private key
* @param _message Message to sign
Expand All @@ -346,7 +320,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
*/
function signData(
address _identity,
address _delegate,
address _registry,
bytes memory _privateKey,
bytes memory _message
Expand All @@ -359,7 +332,7 @@ contract EthereumDIDRegistryDelegateTest is Test {
bytes32 s
)
{
address idOwner = registry.identityOwner(_delegate);
address idOwner = registry.identityOwner(_identity);
uint256 ownerNonce = registry.nonce(idOwner);
return VmDigest.signData(vm, _identity, _registry, _privateKey, _message, ownerNonce);
}
Expand Down Expand Up @@ -389,7 +362,7 @@ contract EthereumDIDRegistryDelegateTest is Test {
)
{
bytes memory message = abi.encodePacked(bytes("addDelegate"), bytes32("attestor"), _delegate, uint256(1 days));
return signData(_identity, _delegate, _registry, _privateKey, message);
return signData(_identity, _registry, _privateKey, message);
}

/**
Expand Down Expand Up @@ -417,6 +390,6 @@ contract EthereumDIDRegistryDelegateTest is Test {
)
{
bytes memory message = abi.encodePacked(bytes("revokeDelegate"), bytes32("attestor"), _delegate);
return signData(_identity, _delegate, _registry, _privateKey, message);
return signData(_identity, _registry, _privateKey, message);
}
}
5 changes: 2 additions & 3 deletions packages/did-eth-registry/test/VmDigest.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,13 @@ library VmDigest {
}

/**
* @dev Convert bytes to uint256
* @dev Convert bytes to uint256. This is intended for test purposes only.
* @param _bytes Bytes to convert
* @return uint256
*/
function toUint256(bytes memory _bytes) internal pure returns (uint256) {
require(_bytes.length >= 32, "toUint256_outOfBounds");
require(_bytes.length <= 32, "toUint256_outOfBounds");
uint256 result;
// for test purposes only
// solhint-disable-next-line no-inline-assembly
assembly {
result := mload(add(_bytes, 0x20))
Expand Down
Loading

0 comments on commit 96dd824

Please sign in to comment.