From 3596ebe15b575b6ecaf51ccc71fa8e072e0c50b5 Mon Sep 17 00:00:00 2001 From: Mikko Ohtamaa Date: Thu, 18 Jan 2024 13:22:55 +0100 Subject: [PATCH 1/3] Emit the full signed message for log records --- src/Counter.sol | 14 -------------- src/TermsOfService.sol | 15 ++++++++++----- 2 files changed, 10 insertions(+), 19 deletions(-) delete mode 100644 src/Counter.sol diff --git a/src/Counter.sol b/src/Counter.sol deleted file mode 100644 index aded799..0000000 --- a/src/Counter.sol +++ /dev/null @@ -1,14 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.13; - -contract Counter { - uint256 public number; - - function setNumber(uint256 newNumber) public { - number = newNumber; - } - - function increment() public { - number++; - } -} diff --git a/src/TermsOfService.sol b/src/TermsOfService.sol index bdb52b4..246f163 100644 --- a/src/TermsOfService.sol +++ b/src/TermsOfService.sol @@ -9,8 +9,6 @@ import "@openzeppelin/access/Ownable.sol"; import "@openzeppelin/utils/cryptography/SignatureChecker.sol"; - - /** * Minimal MVP interface */ @@ -50,7 +48,7 @@ contract TermsOfService is Ownable, ITermsOfService { uint16 public latestTermsOfServiceVersion; // Add a new terms of service version - event UpdateTermsOfService(uint16 version, bytes32 acceptanceMessageHash); + event UpdateTermsOfService(uint16 version, bytes32 acceptanceMessageHash, string acceptanceMessage); event Signed(address signer, uint16 version, bytes32 hash, bytes metadata); @@ -71,13 +69,20 @@ contract TermsOfService is Ownable, ITermsOfService { return hasAcceptedHash(account, hash); } - function updateTermsOfService(uint16 version, bytes32 acceptanceMessageHash) public onlyOwner { + /** + * Update to the new terms of service version. + * + * We do not check whether acceptance message hash matches the message, + * as the message string here is just for the event log keeping. + * + */ + function updateTermsOfService(uint16 version, bytes32 acceptanceMessageHash, string acceptanceMessage) public onlyOwner { require(version == latestTermsOfServiceVersion + 1, "Versions must be updated incrementally"); require(acceptanceMessageHash != latestAcceptanceMessageHash, "Setting the same terms of service twice"); latestAcceptanceMessageHash = acceptanceMessageHash; latestTermsOfServiceVersion = version; versions[version] = acceptanceMessageHash; - emit UpdateTermsOfService(version, acceptanceMessageHash); + emit UpdateTermsOfService(version, acceptanceMessageHash, acceptanceMessage); } /** From c2f18a909a3c38533d252c7fbc2732d495aae83d Mon Sep 17 00:00:00 2001 From: Mikko Ohtamaa Date: Thu, 18 Jan 2024 13:36:18 +0100 Subject: [PATCH 2/3] Bump pragma as older 0.8.x solc could not compile the file --- src/TermsOfService.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/TermsOfService.sol b/src/TermsOfService.sol index 246f163..b47a3bb 100644 --- a/src/TermsOfService.sol +++ b/src/TermsOfService.sol @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.0; +pragma solidity ^0.8.23; import "@openzeppelin/access/Ownable.sol"; @@ -76,7 +76,7 @@ contract TermsOfService is Ownable, ITermsOfService { * as the message string here is just for the event log keeping. * */ - function updateTermsOfService(uint16 version, bytes32 acceptanceMessageHash, string acceptanceMessage) public onlyOwner { + function updateTermsOfService(uint16 version, bytes32 acceptanceMessageHash, string calldata acceptanceMessage) public onlyOwner { require(version == latestTermsOfServiceVersion + 1, "Versions must be updated incrementally"); require(acceptanceMessageHash != latestAcceptanceMessageHash, "Setting the same terms of service twice"); latestAcceptanceMessageHash = acceptanceMessageHash; From 220897d5e10321b83099b4c752d88958801bc2e4 Mon Sep 17 00:00:00 2001 From: Mikko Ohtamaa Date: Thu, 18 Jan 2024 13:43:02 +0100 Subject: [PATCH 3/3] Fix tests to match the new ABI --- README.md | 4 +++- src/TermsOfService.sol | 12 ++++++++++-- tests/test_sign_terms_of_service.py | 10 +++++----- tests/test_update_terms_of_service.py | 14 +++++++------- 4 files changed, 25 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index ffe2595..ad20ad4 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,9 @@ ape compile ```shell poetry shell -ape test +# TODO: there is a bug in pytest/ape test that it does not find the tests +# unless you explicitly pass it the tests folder +ape test tests ``` ## Deploying diff --git a/src/TermsOfService.sol b/src/TermsOfService.sol index b47a3bb..1219b67 100644 --- a/src/TermsOfService.sol +++ b/src/TermsOfService.sol @@ -24,6 +24,7 @@ interface ITermsOfService { */ contract TermsOfService is Ownable, ITermsOfService { + // OpenZeppelin lib supporting smart contract based signing as opposite to EOA signing using SignatureChecker for address; // Terms of service acceptances @@ -37,19 +38,26 @@ contract TermsOfService is Ownable, ITermsOfService { // // Published terms of services // + // + // Terms of service versions, starting from 1 and + // increased with one for the each iteration. + // mapping(uint16 version => bytes32 acceptanceMessageHash) public versions; + // + // What is the hash of currently active terms of service version + // bytes32 public latestAcceptanceMessageHash; // - // Terms of service versions, starting from 1 and - // increased with one for the each iteration. + // What is the version numbe of current terms of service version // uint16 public latestTermsOfServiceVersion; // Add a new terms of service version event UpdateTermsOfService(uint16 version, bytes32 acceptanceMessageHash, string acceptanceMessage); + // User accepted a specific terms of service version event Signed(address signer, uint16 version, bytes32 hash, bytes metadata); constructor() Ownable() { diff --git a/tests/test_sign_terms_of_service.py b/tests/test_sign_terms_of_service.py index b3d341d..68481d1 100644 --- a/tests/test_sign_terms_of_service.py +++ b/tests/test_sign_terms_of_service.py @@ -33,7 +33,7 @@ def test_not_signed( ): new_version = 1 new_hash = random.randbytes(32) - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, "", sender=deployer) assert not tos.hasAcceptedHash(random_user, new_hash) assert not tos.hasAcceptedVersion(random_user, 1) @@ -56,7 +56,7 @@ def test_signed( ) new_hash = get_signing_hash(signing_content) - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, signing_content, sender=deployer) assert not tos.hasAcceptedHash(random_user, new_hash) assert not tos.hasAcceptedVersion(random_user, 1) @@ -94,7 +94,7 @@ def test_sign_twice( ) new_hash = get_signing_hash(signing_content) - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, signing_content, sender=deployer) signature = random_user.sign_message(signing_content).encode_rsv() @@ -120,7 +120,7 @@ def test_sign_no_version( ) new_hash = get_signing_hash(signing_content) - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, signing_content, sender=deployer) signature = random_user.sign_message(signing_content).encode_rsv() @@ -144,7 +144,7 @@ def test_sign_wrong_signature( ) new_hash = get_signing_hash(signing_content) - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, signing_content, sender=deployer) with reverts("Signature is not valid"): tos.signTermsOfServiceOwn(new_hash, random.randbytes(32), b"", sender=random_user) diff --git a/tests/test_update_terms_of_service.py b/tests/test_update_terms_of_service.py index fa8775e..343d96a 100644 --- a/tests/test_update_terms_of_service.py +++ b/tests/test_update_terms_of_service.py @@ -32,7 +32,7 @@ def test_start_zero_version(tos: ContractInstance): def test_first_terms_of_service(tos: ContractInstance, deployer: TestAccount): new_version = 1 new_hash = random.randbytes(32) - tx = tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tx = tos.updateTermsOfService(new_version, new_hash, "", sender=deployer) assert tos.latestTermsOfServiceVersion() == 1 assert tos.latestAcceptanceMessageHash() == new_hash @@ -48,13 +48,13 @@ def test_first_terms_of_service(tos: ContractInstance, deployer: TestAccount): def test_second_terms_of_service(tos: ContractInstance, deployer: TestAccount): new_version = 1 new_hash = random.randbytes(32) - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, "", sender=deployer) assert tos.latestTermsOfServiceVersion() == 1 assert tos.latestAcceptanceMessageHash() == new_hash new_version = 2 new_hash = random.randbytes(32) - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, "", sender=deployer) assert tos.latestTermsOfServiceVersion() == 2 assert tos.latestAcceptanceMessageHash() == new_hash @@ -64,7 +64,7 @@ def test_wrong_owner(tos: ContractInstance, random_user: TestAccount): new_hash = random.randbytes(32) with pytest.raises(ContractLogicError): - tos.updateTermsOfService(new_version, new_hash, sender=random_user) + tos.updateTermsOfService(new_version, new_hash, "", sender=random_user) assert tos.latestTermsOfServiceVersion() == 0 @@ -74,7 +74,7 @@ def test_wrong_version(tos: ContractInstance, deployer: TestAccount): new_hash = random.randbytes(32) with pytest.raises(ContractLogicError): - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, "", sender=deployer) assert tos.latestTermsOfServiceVersion() == 0 @@ -82,11 +82,11 @@ def test_wrong_version(tos: ContractInstance, deployer: TestAccount): def test_hash_reuse(tos: ContractInstance, deployer: TestAccount): new_version = 1 new_hash = random.randbytes(32) - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, "", sender=deployer) new_version = 2 with pytest.raises(ContractLogicError): - tos.updateTermsOfService(new_version, new_hash, sender=deployer) + tos.updateTermsOfService(new_version, new_hash, "", sender=deployer) def test_transfer_ownership(tos: ContractInstance, deployer: TestAccount, random_user: TestAccount):