Feat/eng 483 create jobs #19

gjgd · 2018-07-24T20:21:45Z

https://transmute.atlassian.net/browse/ENG-483

Job manager
- Added category to Mineral struct (either Compute or Storage)
- Added submitJob function
DPOS
- Provider.status was redundant with the containsProvider() function. Removed Provider.status and replaced it with providerStatus that uses containsProvider to determine if Provider is Registered or Unregistered

coveralls · 2018-07-24T20:29:53Z

Pull Request Test Coverage Report for Build 127

26 of 26 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 122:	0.0%
Covered Lines:	94
Relevant Lines:	94

💛 - Coveralls

eolszewski · 2018-07-24T20:44:50Z

contracts/JobManager.sol

@@ -5,21 +5,69 @@ import "zeppelin-solidity/contracts/math/SafeMath.sol";
 contract JobManager {
  using SafeMath for uint;

+  enum MineralCategory { Null, Compute, Storage }


Why do we have a Null category here? Seems unnecessary - the default can be Compute or Storage without any problems.

My idea was to use this to check if a mineral exists for that index.
Given that it is not possible to create a Mineral with Null category, or update it to have Null category, the only way it has Null category is that it does not exist (see mineralIsValid function).

However I like your idea of storing the address who submitted the mineral. This would provide a cleaner way to check if the mineral exists. Will do that

eolszewski · 2018-07-24T20:47:03Z

contracts/JobManager.sol

+    } else if (_category == uint(MineralCategory.Storage)) {
+      mc = MineralCategory.Storage;
+    } else {
+      revert();


Yeah, given that you both cannot create a mineral with category Null and cannot update a mineral to have category Null, I would say that this enum option can be removed.

eolszewski · 2018-07-24T20:58:44Z

contracts/JobManager.sol

+
+  function mineralIsValid(uint _mineralId) public view returns (bool) {
+    Mineral memory m = minerals[_mineralId];
+    return m.category != MineralCategory.Null;


This code does not permit that a mineral to have a Null category. I would update this to verify that a mineral does in fact exist for that index and that maybe that there isn't already a job associated with it.

Naturally, this will be a bidding process with the best priced job winning (we'd think), but we'll assume first-come, first-serve for simplicity purposes, now.

Ex:

Mineral memory m = minerals[_mineralId]; return m.id < numberOfMinerals && m.jobId == 0

I would also call this something like - mineralIsAvailable or something along those lines given that the point is to make sure we can create a job for it. Whenever you create a mineral, the default value of '0' will be assigned to the jobId and index jobs starting from 1.

eolszewski · 2018-07-24T21:05:30Z

contracts/TransmuteDPOS.sol

    p.pricePerStorageMineral = _pricePerStorageMineral;
    p.pricePerComputeMineral = _pricePerComputeMineral;
    p.blockRewardCut = _blockRewardCut;
    p.feeShare = _feeShare;
  }

  function resignAsProvider(address _provider) internal {
-    require(providers[_provider].status != ProviderStatus.Unregistered);
+    require(providerStatus(_provider) != ProviderStatus.Unregistered);


I feel like this should check to ensure that the provider is Registered vs not Unregistered, for readability purposes.

eolszewski · 2018-07-24T21:13:18Z

contracts/TransmuteDPOS.sol

@@ -112,14 +110,15 @@ contract TransmuteDPOS is TransmuteToken, RoundManager, ProviderPool {
    Provider storage p = providers[_provider];
    // A delegator is only allowed to bond to himself (in which case he wants to be a Provider)
    // or to a Registered Provider
-    require(_provider == msg.sender || p.status == ProviderStatus.Registered);
+    ProviderStatus pStatus = providerStatus(_provider);
+    require(_provider == msg.sender || pStatus == ProviderStatus.Registered);


Why have a temp variable here and not just use the function in the equality check like you did on line 102?

eolszewski · 2018-07-24T21:36:39Z

test/JobManager.spec.js

+    it('should fail if category is not MINERAL_COMPUTE or MINERAL_STORAGE', async () => {
+      await assertFail( jm.submitMineral("test", 0) );
+      await jm.submitMineral("test", MINERAL_COMPUTE);
+      await jm.submitMineral("test", MINEARL_STORAGE);


MINERAL_STORAGE*

eolszewski · 2018-07-24T21:37:41Z

test/JobManager.spec.js

 require('truffle-test-utils').init();

 contract('JobManager', accounts => {

  let jm;
+  const MINERAL_COMPUTE = 1;


These will need to be updated to 0 and 1

eolszewski · 2018-07-24T21:39:17Z

test/JobManager.spec.js

+
+    it('should accept no name for Mineral', async () => {
+      const mineralId = (await jm.numberOfMinerals.call()).toNumber();
+      await jm.submitMineral("", MINERAL_COMPUTE);


We should probably be associating minerals with the users who submitted them and requiring a non-empty string for a name. If I had created a few minerals, how I would I be able to search them with this architecture?

eolszewski · 2018-07-24T21:39:49Z

test/JobManager.spec.js

+      });
+    });
+
+    it('should accept no name for Mineral', async () => {


I don't agree with permitting this to be the case. Explanation below.

eolszewski · 2018-07-24T21:40:13Z

test/JobManager.spec.js

+      const mineralId = (await jm.numberOfMinerals.call()).toNumber();
+      await jm.submitMineral("", MINERAL_COMPUTE);
+      const mineral = await jm.minerals.call(mineralId);
+      let [name, _] = mineral;


_ is not a reasonable name for a variable.

gjgd added 10 commits July 24, 2018 17:38

Add job function

2346cec

Add tests

945cbac

Fix test

cfef7d0

Add mineral category

d6f798a

Fix test to include mineral category

c688fab

Add test for mineral category

4be3764

Change the definition of mineralIsValid using mineralId

2d63dc6

Add test about name parameter

a50a55c

Replace status parameter in Provider struct by providerStatus method

18f5265

Change name of job function

99ab8bf

gjgd requested review from eolszewski and OR13 July 24, 2018 20:27

eolszewski suggested changes Jul 24, 2018

View reviewed changes

gjgd added 6 commits July 25, 2018 13:29

Fix typo in const name

075c16f

Change variable name

97a4507

Add producer address to Mineral struct

4f39987

Remove Null category

02fe650

Enforce that name should be a non empty string

79f8ec2

Remove double negation in condition

2be7025

gjgd merged commit 7483331 into master Jul 25, 2018

gjgd deleted the feat/ENG-483-create-jobs branch July 25, 2018 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/eng 483 create jobs #19

Feat/eng 483 create jobs #19

gjgd commented Jul 24, 2018 •

edited

coveralls commented Jul 24, 2018 •

edited

eolszewski Jul 24, 2018

gjgd Jul 25, 2018

eolszewski Jul 24, 2018

eolszewski Jul 24, 2018

eolszewski Jul 24, 2018

eolszewski Jul 24, 2018

eolszewski Jul 24, 2018

eolszewski Jul 24, 2018

eolszewski Jul 24, 2018

eolszewski Jul 24, 2018

eolszewski Jul 24, 2018

Feat/eng 483 create jobs #19

Feat/eng 483 create jobs #19

Conversation

gjgd commented Jul 24, 2018 • edited

coveralls commented Jul 24, 2018 • edited

Pull Request Test Coverage Report for Build 127

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjgd commented Jul 24, 2018 •

edited

coveralls commented Jul 24, 2018 •

edited