Skip to content

Commit

Permalink
Block Proposal Bug Fix (#347)
Browse files Browse the repository at this point in the history 
* Block Proposal Bug Fix

* Linting Fixes

* Test Fixes

* test fixes & skanda's changes

* CollectionCreated event removed
  • Loading branch information
@SamAg19
SamAg19 authored Aug 25, 2021
1 parent eb2bd74 commit ac97f68
Show file tree
Hide file tree
Showing 12 changed files with 218 additions and 101 deletions.
81 changes: 33 additions & 48 deletions contracts/Core/AssetManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,36 +4,13 @@ pragma solidity ^0.8.0;
import "./interface/IParameters.sol";
import "./storage/AssetStorage.sol";
import "./storage/Constants.sol";
import "./StateManager.sol";
import "./ACL.sol";

contract AssetManager is ACL, AssetStorage, Constants {
contract AssetManager is ACL, AssetStorage, Constants, StateManager {
IParameters public parameters;

event JobCreated(
bool repeat,
uint8 id,
int8 power,
address creator,
uint32 epoch,
uint256 timestamp,
string name,
string selector,
string url
);

event JobReported(
bool active,
bool repeat,
uint8 id,
int8 power,
address creator,
uint32 value,
uint32 epoch,
uint256 timestamp,
string name,
string selector,
string url
);
event JobCreated(uint8 id, int8 power, address creator, uint32 epoch, uint256 timestamp, string name, string selector, string url);

event JobUpdated(uint8 id, uint32 epoch, int8 power, uint256 timestamp, string selector, string url);

Expand All @@ -51,18 +28,6 @@ contract AssetManager is ACL, AssetStorage, Constants {
string name
);

event CollectionReported(
uint8 id,
int8 power,
uint32 epoch,
uint32 aggregationMethod,
uint32 value,
uint8[] jobIDs,
address creator,
uint256 timestamp,
string name
);

event CollectionUpdated(
uint8 id,
uint32 epoch,
Expand All @@ -80,7 +45,6 @@ contract AssetManager is ACL, AssetStorage, Constants {
}

function createJob(
bool repeat,
int8 power,
string calldata name,
string calldata selector,
Expand All @@ -89,17 +53,17 @@ contract AssetManager is ACL, AssetStorage, Constants {
numAssets = numAssets + 1;
uint32 epoch = parameters.getEpoch();

jobs[numAssets] = Structs.Job(true, repeat, numAssets, uint8(assetTypes.Job), power, epoch, msg.sender, name, selector, url);
jobs[numAssets] = Structs.Job(true, numAssets, uint8(assetTypes.Job), power, epoch, msg.sender, name, selector, url);

emit JobCreated(repeat, numAssets, power, msg.sender, epoch, block.timestamp, name, selector, url);
emit JobCreated(numAssets, power, msg.sender, epoch, block.timestamp, name, selector, url);
}

function updateJob(
uint8 jobID,
int8 power,
string calldata selector,
string calldata url
) external onlyRole(ASSET_MODIFIER_ROLE) {
) external onlyRole(ASSET_MODIFIER_ROLE) notCommitState(State.Commit, parameters.epochLength()) {
require(jobs[jobID].assetType == uint8(assetTypes.Job), "Job ID not present");

uint32 epoch = parameters.getEpoch();
Expand All @@ -110,7 +74,11 @@ contract AssetManager is ACL, AssetStorage, Constants {
emit JobUpdated(jobID, epoch, power, block.timestamp, selector, url);
}

function setAssetStatus(bool assetStatus, uint8 id) external onlyRole(ASSET_MODIFIER_ROLE) {
function setAssetStatus(bool assetStatus, uint8 id)
external
onlyRole(ASSET_MODIFIER_ROLE)
checkDisputeOrConfirmState(State.Dispute, State.Confirm, parameters.epochLength())
{
require(id != 0, "ID cannot be 0");

require(id <= numAssets, "ID does not exist");
Expand All @@ -123,6 +91,11 @@ contract AssetManager is ACL, AssetStorage, Constants {
emit JobActivityStatus(jobs[id].active, id, epoch, block.timestamp);
} else {
collections[id].active = assetStatus;
if (assetStatus) {
numActiveAssets = numActiveAssets + 1;
} else {
numActiveAssets = numActiveAssets - 1;
}

emit CollectionActivityStatus(collections[id].active, id, epoch, block.timestamp);
}
Expand All @@ -133,12 +106,13 @@ contract AssetManager is ACL, AssetStorage, Constants {
uint32 aggregationMethod,
int8 power,
string calldata name
) external onlyRole(ASSET_MODIFIER_ROLE) {
) external onlyRole(ASSET_MODIFIER_ROLE) checkDisputeOrConfirmState(State.Dispute, State.Confirm, parameters.epochLength()) {
require(aggregationMethod > 0 && aggregationMethod < parameters.aggregationRange(), "Aggregation range out of bounds");

require(jobIDs.length > 1, "Number of jobIDs low to create collection");

numAssets = numAssets + 1;
numActiveAssets = numActiveAssets + 1;
uint32 epoch = parameters.getEpoch();

collections[numAssets].id = numAssets;
Expand All @@ -163,7 +137,11 @@ contract AssetManager is ACL, AssetStorage, Constants {
emit CollectionCreated(true, numAssets, power, epoch, aggregationMethod, jobIDs, msg.sender, block.timestamp, name);
}

function addJobToCollection(uint8 collectionID, uint8 jobID) external onlyRole(ASSET_MODIFIER_ROLE) {
function addJobToCollection(uint8 collectionID, uint8 jobID)
external
onlyRole(ASSET_MODIFIER_ROLE)
notCommitState(State.Commit, parameters.epochLength())
{
require(collections[collectionID].assetType == uint8(assetTypes.Collection), "Collection ID not present");

require(collections[collectionID].active, "Collection is inactive");
Expand Down Expand Up @@ -191,7 +169,11 @@ contract AssetManager is ACL, AssetStorage, Constants {
);
}

function removeJobFromCollection(uint8 collectionID, uint8 jobIDIndex) external onlyRole(ASSET_MODIFIER_ROLE) {
function removeJobFromCollection(uint8 collectionID, uint8 jobIDIndex)
external
onlyRole(ASSET_MODIFIER_ROLE)
notCommitState(State.Commit, parameters.epochLength())
{
require(collections[collectionID].assetType == uint8(assetTypes.Collection), "Collection ID not present");

require(collections[collectionID].jobIDs.length > jobIDIndex, "Index not in range");
Expand Down Expand Up @@ -255,7 +237,6 @@ contract AssetManager is ACL, AssetStorage, Constants {
view
returns (
bool active,
bool repeat,
int8 power,
string memory name,
string memory selector,
Expand All @@ -265,7 +246,7 @@ contract AssetManager is ACL, AssetStorage, Constants {
require(jobs[id].assetType == uint8(assetTypes.Job), "ID is not a job");

Structs.Job memory job = jobs[id];
return (job.active, job.repeat, job.power, job.name, job.selector, job.url);
return (job.active, job.power, job.name, job.selector, job.url);
}

function getCollection(uint8 id)
Expand Down Expand Up @@ -317,4 +298,8 @@ contract AssetManager is ACL, AssetStorage, Constants {
function getNumAssets() external view returns (uint8) {
return numAssets;
}

function getNumActiveAssets() external view returns (uint8) {
return numActiveAssets;
}
}
2 changes: 1 addition & 1 deletion contracts/Core/BlockManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,10 @@ contract BlockManager is Initializable, ACL, BlockStorage, StateManager {
uint32 proposerId = stakeManager.getStakerId(msg.sender);
require(isElectedProposer(iteration, biggestInfluencerId, proposerId), "not elected");
require(stakeManager.getStake(proposerId) >= parameters.minStake(), "stake below minimum stake");

//staker can just skip commit/reveal and only propose every epoch to avoid penalty.
//following line is to prevent that
require(voteManager.getEpochLastRevealed(proposerId) == epoch, "Cannot propose without revealing");
require(medians.length == assetManager.getNumActiveAssets(), "invalid block proposed");

uint256 biggestInfluence = stakeManager.getInfluence(biggestInfluencerId);
uint8 numProposedBlocks = uint8(sortedProposedBlockIds[epoch].length);
Expand Down
4 changes: 2 additions & 2 deletions contracts/Core/Parameters.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ contract Parameters is ACL, Constants {
}

function getState() external view returns (uint8) {
uint8 state = uint8(((block.number) / (epochLength / NUM_STATES)));
return (state % (NUM_STATES));
uint8 state = uint8(((block.number) / (epochLength / NUM_STATES)) % (NUM_STATES));
return (state);
}
}
14 changes: 14 additions & 0 deletions contracts/Core/StateManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@ contract StateManager is Constants {
_;
}

modifier checkDisputeOrConfirmState(
State dispute,
State confirm,
uint32 epochLength
) {
require(dispute == getState(epochLength) || confirm == getState(epochLength), "incorrect state");
_;
}

modifier notCommitState(State state, uint32 epochLength) {
require(state != getState(epochLength), "incorrect state");
_;
}

modifier checkEpochAndState(
State state,
uint32 epoch,
Expand Down
2 changes: 2 additions & 0 deletions contracts/Core/interface/IAssetManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,6 @@ interface IAssetManager {
uint32 aggregationMethod,
string memory name
);

function getNumActiveAssets() external view returns (uint8);
}
1 change: 1 addition & 0 deletions contracts/Core/storage/AssetStorage.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ contract AssetStorage {
mapping(uint8 => Structs.Collection) public collections;

uint8 public numAssets;
uint8 public numActiveAssets;
}
1 change: 0 additions & 1 deletion contracts/lib/Structs.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ library Structs {

struct Job {
bool active;
bool repeat;
uint8 id;
uint8 assetType;
int8 power;
Expand Down
52 changes: 34 additions & 18 deletions test/ACL.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const {
ASSET_MODIFIER_ROLE,
} = require('./helpers/constants');
const {
assertRevert, restoreSnapshot, takeSnapshot, waitNBlocks,
assertRevert, restoreSnapshot, takeSnapshot, waitNBlocks, mineToNextState,
} = require('./helpers/testHelpers');
const { setupContracts } = require('./helpers/testSetup');
const {
Expand Down Expand Up @@ -204,27 +204,27 @@ describe('Access Control Test', async () => {

it('createJob() should not be accessable by anyone besides AssetCreator', async () => {
// Checking if Anyone can access it
await assertRevert(assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1'), expectedRevertMessage);
await assertRevert(assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1'), expectedRevertMessage);

// Checking if BlockConfirmer can access it
await assetManager.grantRole(BLOCK_CONFIRMER_ROLE, signers[0].address);
await assertRevert(assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1'), expectedRevertMessage);
await assertRevert(assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1'), expectedRevertMessage);

// Checking if StakeModifier can access it
await assetManager.grantRole(STAKE_MODIFIER_ROLE, signers[0].address);
await assertRevert(assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1'), expectedRevertMessage);
await assertRevert(assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1'), expectedRevertMessage);

// Checking if StakerActivityUpdater can access it
await assetManager.grantRole(STAKER_ACTIVITY_UPDATER_ROLE, signers[0].address);
await assertRevert(assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1'), expectedRevertMessage);
await assertRevert(assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1'), expectedRevertMessage);
});

it('createJob() should be accessable by only AssetCreator', async () => {
const assetCreatorHash = ASSET_MODIFIER_ROLE;
await assetManager.grantRole(assetCreatorHash, signers[0].address);
await assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.revokeRole(assetCreatorHash, signers[0].address);
await assertRevert(assetManager.createJob(true, 0, 'http://testurl.com/2', 'selector/2', 'test2'), expectedRevertMessage);
await assertRevert(assetManager.createJob(0, 'http://testurl.com/2', 'selector/2', 'test2'), expectedRevertMessage);
});

it('updateJob() should not be accessable by anyone besides AssetCreator', async () => {
Expand All @@ -247,7 +247,8 @@ describe('Access Control Test', async () => {
it('updateJob() should be accessable by only AssetCreator', async () => {
const assetCreatorHash = ASSET_MODIFIER_ROLE;
await assetManager.grantRole(assetCreatorHash, signers[0].address);
await assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1');
await mineToNextState();
await assetManager.updateJob(1, 2, 'http://testurl.com/2', 'selector/2');
await assetManager.revokeRole(assetCreatorHash, signers[0].address);
await assertRevert(assetManager.updateJob(1, 2, 'http://testurl.com/2', 'selector/2'), expectedRevertMessage);
Expand All @@ -273,7 +274,10 @@ describe('Access Control Test', async () => {
it('setAssetStatus() should be accessable by only AssetCreator', async () => {
const assetCreatorHash = ASSET_MODIFIER_ROLE;
await assetManager.grantRole(assetCreatorHash, signers[0].address);
await assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1');
await mineToNextState();
await mineToNextState();
await mineToNextState();
await assetManager.setAssetStatus(true, 1);
await assetManager.revokeRole(assetCreatorHash, signers[0].address);
await assertRevert(assetManager.setAssetStatus(true, 1), expectedRevertMessage);
Expand All @@ -299,8 +303,11 @@ describe('Access Control Test', async () => {
it('createCollection() should be accessable by only AssetCreator', async () => {
const assetCreatorHash = ASSET_MODIFIER_ROLE;
await assetManager.grantRole(assetCreatorHash, signers[0].address);
await assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(true, 0, 'http://testurl.com/2', 'selector/2', 'test2');
await assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(0, 'http://testurl.com/2', 'selector/2', 'test2');
await mineToNextState();// reveal
await mineToNextState();// propose
await mineToNextState();// dispute
await assetManager.createCollection([1, 2], 1, 0, 'test');
await assetManager.revokeRole(assetCreatorHash, signers[0].address);
await assertRevert(assetManager.createCollection([1, 2], 1, 0, 'test'), expectedRevertMessage);
Expand All @@ -326,10 +333,13 @@ describe('Access Control Test', async () => {
it('addJobToCollection() should be accessable by only AssetCreator', async () => {
const assetCreatorHash = ASSET_MODIFIER_ROLE;
await assetManager.grantRole(assetCreatorHash, signers[0].address);
await assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(true, 0, 'http://testurl.com/2', 'selector/2', 'test2');
await assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(0, 'http://testurl.com/2', 'selector/2', 'test2');
await mineToNextState();// reveal
await mineToNextState();// propose
await mineToNextState();// dispute
await assetManager.createCollection([1, 2], 1, 0, 'test');
await assetManager.createJob(true, 0, 'http://testurl.com/3', 'selector/3', 'test3');
await assetManager.createJob(0, 'http://testurl.com/3', 'selector/3', 'test3');
await assetManager.addJobToCollection(3, 4);
await assetManager.revokeRole(assetCreatorHash, signers[0].address);
await assertRevert(assetManager.addJobToCollection(3, 4), expectedRevertMessage);
Expand All @@ -355,8 +365,11 @@ describe('Access Control Test', async () => {
it('removeJobFromCollection() should be accessable by only AssetCreator', async () => {
const assetCreatorHash = ASSET_MODIFIER_ROLE;
await assetManager.grantRole(assetCreatorHash, signers[0].address);
await assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(true, 0, 'http://testurl.com/2', 'selector/2', 'test2');
await assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(0, 'http://testurl.com/2', 'selector/2', 'test2');
await mineToNextState();// reveal
await mineToNextState();// propose
await mineToNextState();// dispute
await assetManager.createCollection([1, 2], 1, 0, 'test');
await assetManager.removeJobFromCollection(3, 1);
await assetManager.revokeRole(assetCreatorHash, signers[0].address);
Expand Down Expand Up @@ -384,8 +397,11 @@ describe('Access Control Test', async () => {
const assetModifierHash = ASSET_MODIFIER_ROLE;
await assetManager.grantRole(assetModifierHash, signers[0].address);

await assetManager.createJob(true, 0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(true, 0, 'http://testurl.com/2', 'selector/2', 'test2');
await assetManager.createJob(0, 'http://testurl.com/1', 'selector/1', 'test1');
await assetManager.createJob(0, 'http://testurl.com/2', 'selector/2', 'test2');
await mineToNextState();// reveal
await mineToNextState();// propose
await mineToNextState();// dispute
await assetManager.createCollection([1, 2], 1, 0, 'test');

await assetManager.updateCollection(3, 2, -2);
Expand Down
Loading

0 comments on commit ac97f68

Please sign in to comment.