feat/policiesForRecords #108

MaxBlaushild · 2018-10-29T14:19:31Z

Implements policy constrained record appending and persistent record based policies.

Users now have the option to check against policies when appending data. These policies are stored, and again checked when user grants access for file to new users.

Also allows users to add and remove policies for their records.

codecov-io · 2018-10-29T14:31:16Z

Codecov Report

Merging #108 into master will decrease coverage by 1.84%.
The diff coverage is 87.75%.

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
- Coverage    96.6%   94.75%   -1.85%     
==========================================
  Files           8        9       +1     
  Lines         206      248      +42     
  Branches       48       57       +9     
==========================================
+ Hits          199      235      +36     
- Misses          7       13       +6

Impacted Files	Coverage Δ
contracts/mock/PolicyMock.sol	`100% <ø> (ø)`
contracts/LinniaPermissions.sol	`98.03% <100%> (-0.18%)`	⬇️
contracts/LinniaHub.sol	`95.45% <100%> (+1.33%)`	⬆️
contracts/LinniaPolicies.sol	`80% <80%> (ø)`
contracts/LinniaRecords.sol	`97.11% <90.9%> (-1.67%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f2b1a5...ad50229. Read the comment docs.

contracts/interfaces/PolicyI.sol

godfreyhobbs · 2018-10-30T22:03:15Z

test/4_permissions_test.js

-        'sender': user1
-      };
-
-      assert.equal(tx.logs[0].event, 'LinniaPolicyChecked');


It would be good to check for the new event logs.

The new event logs aren't available in the returned transaction logs. I test for them in the method that emits them though

migrations/2_deploy_contracts.js

godfreyhobbs · 2018-10-30T22:04:48Z

contracts/mock/PolicyMock.sol



-contract /* interface */ PermissionPolicyMock is PermissionPolicyI {
+contract /* interface */ PolicyMock is PolicyI {


remove /* interface */

contracts/LinniaRecords.sol

godfreyhobbs

I don't think this makes sense to have the policies in a different contract. If we update the new policy contract we will remove all of the records policies. This seems to be a security hole.

I think this all needs to be in the same contract until we have a way of migrating data when contracts are updated.

godfreyhobbs · 2018-11-14T22:57:35Z

test/3_records_test.js

 const LinniaRecords = artifacts.require('./LinniaRecords.sol');
+const LinniaPolicies = artifacts.require('./LinniaPolicies.sol');
 const irisScoreProvider = artifacts.require('./mock/IrisScoreProviderMock.sol');
+const Policy = artifacts.require('./mock/PolicyMock.sol');


consider naming PolicyMock

godfreyhobbs · 2018-11-14T23:04:07Z

consider adding the following to the 3_records_test.js and 5_records-token_test.js

  beforeEach('set up a LinniaPolicies contract', async () => {
    policiesInstance = await LinniaPolicies.new(hub.address);
    await hub.setPoliciesContract(policiesInstance.address);
  });
```

godfreyhobbs · 2018-11-14T23:07:35Z

contracts/LinniaPolicies.sol

+    /* @param viewer the address of the person being given access the record */
+    /* @param dataUri where the record is */
+    /* @param policies an array of the policy conforming addresses */
+    function policyIsValid(


I think this should be internal

godfreyhobbs · 2018-11-14T23:08:01Z

contracts/LinniaPolicies.sol

+    /* @param dataHash the dataHash of the record being checked */
+    /* @param viewer the address of the person being given access the record */
+    /* @param dataUri where the record is */
+    function followsExistingPolicies(


I think this should be internal

Agreed. I'll change

godfreyhobbs · 2018-11-14T23:08:09Z

contracts/LinniaPolicies.sol

+    /* @param viewer the address of the person being given access the record */
+    /* @param dataUri where the record is */
+    /* @param policies an array of the policy conforming addresses */
+    function policiesAreValid(


I think this should be internal

People may want to reveal to people which policies their records and permissions can/will conform with from a UI. Keeping public also allows people to surface better error messages from the UI

godfreyhobbs · 2018-11-14T23:10:30Z

contracts/LinniaPolicies.sol

+        public
+        returns (bool)
+    {
+        return policiesAreValid(


why not inline this method. Seems easier to be calling policiesAreValid directly rather than using this wrapper and spending gas.

godfreyhobbs · 2018-11-14T23:13:33Z

contracts/LinniaRecords.sol


+    /* @dev allows a record owner to add a policy to a record as long as it conforms */
+    /* @param dataHash the record the owner is adding a policy to */
+    /* @param policy an address of a contract that conforms to the policy interface to add */


consider rewording:
an address of a contract that conforms to the policy interface to add

godfreyhobbs · 2018-11-14T23:20:01Z

contracts/LinniaRecords.sol

+        require(hub.policiesContract().policyIsValid(
+            dataHash,
+            msg.sender,
+            records[dataHash].dataUri,


I am not sure why this check is needed. Can you explain?

godfreyhobbs · 2018-11-14T23:22:33Z

contracts/LinniaRecords.sol

+            policy
+        ));
+
+        policies[dataHash].push(policy);


A few things:

It would be good to revert if the policy has already been added.

Also would be good to check for address(0)

It would be good to check that this address conforms to the interface (not sure how todo this)

godfreyhobbs · 2018-11-14T23:23:47Z

contracts/LinniaRecords.sol

+        external
+        returns (bool)
+    {
+        /* @dev search through the policies for the record */


consider reverting if nothing is deleted.

godfreyhobbs · 2018-11-14T23:24:32Z

contracts/LinniaRecords.sol

+        returns (bool)
+    {
+        /* @dev search through the policies for the record */
+        for (uint i = 0; i < policies[dataHash].length; i++) {


consider saving policies[dataHash].length to local var. This may be doing the lookup for every loop.

godfreyhobbs · 2018-11-14T23:25:01Z

contracts/LinniaRecords.sol

+            }
+        }
+
+        return true;


I would only return true if something was actually deleted.

godfreyhobbs · 2018-11-14T23:26:06Z

contracts/LinniaRecords.sol

+            /* @dev if one is found */
+            if (policies[dataHash][i] == policy) {
+                /* @dev remove the policy by shifting everything over and shortening array */
+                while (i < policies[dataHash].length - 1) {


use the swap method here it will use much less gas.

godfreyhobbs · 2018-11-14T23:26:38Z

contracts/LinniaRecords.sol

+                    i++;
+                }
+
+                policies[dataHash].length--;


move return true here.

godfreyhobbs · 2018-11-14T23:28:48Z

contracts/LinniaRecords.sol

+        returns (bool)
+    {
+
+        require(_addRecord(dataHash, msg.sender, metadata, dataUri));


Should this come after the policy check? Please add a comment on why this is first.

godfreyhobbs · 2018-11-14T23:29:27Z

contracts/LinniaRecords.sol

+        require(_addRecord(dataHash, msg.sender, metadata, dataUri));
+
+        for (uint i = 0; i < newPolicies.length; i++) {
+            /* @dev make sure the record conforms with each of the policies,then add*/


comment suggest adding is after this but it is before.

feat/policiesForRecords

e2c7363

MaxBlaushild requested review from anujshah108, davidrhodus, godfreyhobbs, tesla809 and vrotmanh October 29, 2018 14:19

MaxBlaushild added 3 commits October 29, 2018 10:20

fixes linting

4c87a25

more linting

74dec88

makes iris score test past on circle

47ab41e

anujshah108 approved these changes Oct 30, 2018

View reviewed changes

godfreyhobbs reviewed Oct 30, 2018

View reviewed changes

contracts/interfaces/PolicyI.sol Outdated Show resolved Hide resolved

godfreyhobbs reviewed Oct 30, 2018

View reviewed changes

migrations/2_deploy_contracts.js Show resolved Hide resolved

godfreyhobbs reviewed Oct 30, 2018

View reviewed changes

contracts/LinniaRecords.sol Outdated Show resolved Hide resolved

godfreyhobbs suggested changes Oct 30, 2018

View reviewed changes

MaxBlaushild added 2 commits November 2, 2018 12:16

moves record policies to records contract

57470d6

linting

ad50229

MaxBlaushild force-pushed the feat/policiesForRecords branch from 836308a to ad50229 Compare November 2, 2018 16:24

godfreyhobbs reviewed Nov 14, 2018

View reviewed changes

contracts/LinniaRecords.sol

}

}

return true;

Copy link

Contributor

godfreyhobbs Nov 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would only return true if something was actually deleted.

godfreyhobbs reviewed Nov 14, 2018

View reviewed changes

contracts/LinniaRecords.sol

i++;

}

policies[dataHash].length--;

Copy link

Contributor

godfreyhobbs Nov 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move return true here.

godfreyhobbs reviewed Nov 14, 2018

View reviewed changes



		contract /* interface */ PermissionPolicyMock is PermissionPolicyI {
		contract /* interface */ PolicyMock is PolicyI {

feat/policiesForRecords #108

Are you sure you want to change the base?

feat/policiesForRecords #108

Uh oh!

Conversation

MaxBlaushild commented Oct 29, 2018

Uh oh!

codecov-io commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

godfreyhobbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godfreyhobbs commented Nov 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Oct 29, 2018 •

edited

Loading