Asset deactivation does not cause age penalty issue anymore #629

SamAg19 · 2021-12-23T11:38:31Z

fixes #626

coveralls · 2021-12-23T11:42:28Z

Coverage decreased (-0.2%) to 96.489% when pulling 648fe12 on SamAg19:penaltyBug into 14126e8 on razor-network:master.

GasCheckBot · 2021-12-23T11:45:31Z

Contract	Method	Current(Maximum)	Master(Maximum)	Change%(Maximum)	Diff(Maximum)	Current(Minimum)	Master(Minimum)	Diff(Minimum)	Change%(Minimum)
BlockManager	claimBlockReward	691019	268862	+157.02	+422157.00	68369	49915	+18454.00	+36.97
BlockManager	confirmPreviousEpochBlock	41559	42083	-1.25	-524	41559	42083	-524	-1.25
BlockManager	disputeBiggestStakeProposed	145266	145288	-0.02	-22	145266	145288	-22	-0.02
BlockManager	finalizeDispute	171271	179396	-4.53	-8125	148048	156173	-8125	-5.2
BlockManager	giveSorted	184867	185735	-0.47	-868	62395	63066	-671	-1.06
BlockManager	propose	287901	287961	-0.02	-60	155764	155824	-60	-0.04
BlockManager	resetDispute	33429	33412	+0.05	+17.00	33429	33412	+17.00	+0.05
BlockManager	revokeRole	29371	29393	-0.07	-22	29371	29393	-22	-0.07
RewardManager	givePenalties	75140	75206	-0.09	-66	75140	75206	-66	-0.09
RewardManager	revokeRole	29398	29420	-0.07	-22	29398	29420	-22	-0.07
StakeManager	delegate	135951	135928	+0.02	+23.00	96939	96916	+23.00	+0.02
StakeManager	pause	46997	46975	+0.05	+22.00	46997	46975	+22.00	+0.05
StakeManager	revokeRole	29393	29415	-0.07	-22	29009	29031	-22	-0.08
StakeManager	setDelegationAcceptance	30821	30862	-0.13	-41	30821	30862	-41	-0.13
StakeManager	stake	1455924	1455969	-0	-45	91250	91295	-45	-0.05
StakeManager	unstake	174826	174870	-0.03	-44	133302	133346	-44	-0.03
VoteManager	commit	588974	435535	+35.23	+153439.00	114887	114963	-76	-0.07
VoteManager	reveal	246597	239068	+3.15	+7529.00	135597	128068	+7529.00	+5.88
Governance	disableEscapeHatch	40689	40666	+0.06	+23.00	40689	40666	+23.00	+0.06
Governance	setEpochLength	117202	117174	+0.02	+28.00	117202	117174	+28.00	+0.02
Governance	setEpochLimitForUpdateCommission	39062	39018	+0.11	+44.00	39062	39018	+44.00	+0.11
Governance	setMaxAltBlocks	41111	41090	+0.05	+21.00	41111	41090	+21.00	+0.05
Governance	setMaxCommission	41135	41157	-0.05	-22	41135	41157	-22	-0.05
Governance	setMaxTolerance	51752	51706	+0.09	+46.00	51752	51706	+46.00	+0.09
Governance	setMinStake	117480	117413	+0.06	+67.00	52925	52872	+53.00	+0.10
Governance	setWithdrawLockPeriod	41112	41090	+0.05	+22.00	41112	41090	+22.00	+0.05
Governance	setWithdrawReleasePeriod	41137	41114	+0.06	+23.00	41137	41114	+23.00	+0.06
Delegator	grantRole	51283	51305	-0.04	-22	51271	51293	-22	-0.04
Delegator	updateAddress	68887	68842	+0.07	+45.00	34663	34618	+45.00	+0.13
RandomNoManager	initialize	73897	73831	+0.09	+66.00	73897	73831	+66.00	+0.09

SamAg19 · 2021-12-23T12:32:06Z

contracts/Core/AssetManager.sol

-                collections[id].assetIndex = uint16(activeCollections.length);
+                // slither-disable-next-line incorrect-equality
+                if (updateRegistry == epoch) {
+                    // slither-disable-next-line reentrancy-events,reentrancy-no-eth,reentrancy-benign


I have disabled the reentrancy slither issues on updateRegistry functions for now but we will need to address this.
If we shift the registry from the delegator to the asset Manager, the reentrancy issues can be fixed.

@hrishikeshio @abhishekvispute @SkandaBhat

0xcuriousapple · 2021-12-24T07:52:50Z

contracts/Core/AssetManager.sol


-        emit AssetCreated(AssetType.Job, numAssets, block.timestamp);
-        delegator.setIDName(name, numAssets);
+        emit AssetCreated(AssetType.Job, numJobs, block.timestamp);


I think lets discard this assetDefination only throughout the code
It creates confusion without adding any value now
Let it be collection and job everywhere
What do you think ?

Hmm one thing
I think we should merge this change after releasing version for Public Incent Testnet
as this is going to impact client side as well as razorScan

also this AssetType its not needed anymore I think with this PR

enum maybe required for the client side because id 1 can be a job or collection. Lets discuss more on this

0xcuriousapple · 2021-12-24T08:22:04Z

contracts/Core/AssetManager.sol

@@ -82,37 +81,28 @@ contract AssetManager is AssetStorage, StateManager, AssetManagerParams, IAssetM
        uint32 epoch = _getEpoch(epochLength);
        if (assetStatus) {
            if (!collections[id].active) {
-                activeCollections.push(id);
-                collections[id].assetIndex = uint16(activeCollections.length);
+                // slither-disable-next-line incorrect-equality


Are lines like L79 needed anymore, now we are not sharing same space for collection and jobs, so I think it can be removed or else it can be replaced by id <= numCollections

I'll replace it with id <= numCollections

0xcuriousapple · 2021-12-24T08:23:26Z

contracts/Core/AssetManager.sol

                collections[id].active = assetStatus;
+                updateRegistry = epoch + 1;


hey what does updateRegistry mean ?

Epoch in which the registry needs to be updated. I think i should rename to updateRegistryEpoch

SamAg19 · 2022-01-10T10:17:32Z

contracts/Core/BlockManager.sol

-        address randomNoManagerAddress
+        address collectionManagerAddress,
+        address randomNoManagerAddress,
+        address delegatorAddress


Needs to be removed

SamAg19 · 2022-01-10T10:26:26Z

contracts/Core/BlockManager.sol

-            assetManager.executePendingDeactivations(epoch);
+            uint32 updateRegistryEpoch = collectionManager.getUpdateRegistryEpoch();
+            // slither-disable-next-line incorrect-equality
+            if (updateRegistryEpoch == epoch) {


change from == to <= and add another condition of updateRegistry != 0

SamAg19 · 2022-01-10T10:32:31Z

contracts/Core/BlockManager.sol

        if (sortedProposedBlockIds[epoch - 1].length == 0 || blockIndexToBeConfirmed == -1) {
-            assetManager.executePendingDeactivations(epoch);
+            uint32 updateRegistryEpoch = collectionManager.getUpdateRegistryEpoch();


registry needs to be updated irrespective of valid block. hence, shift it outside the if block

SamAg19 · 2022-01-10T10:40:54Z

contracts/Core/CollectionManager.sol

+    }
+
+    function _updateRegistry() internal {
+        uint8 j = 1;


change it to uint16

SamAg19 · 2022-01-10T10:51:43Z

contracts/Core/CollectionManager.sol

        checkState(State.Confirm, epochLength)
    {
        require(id != 0, "ID cannot be 0");
-        require(collections[id].id == id, "Asset is not a collection");
+        require(id <= numCollections, "ID does not exist");



require for the assetStatus to be opposite of collections[id].active. allows to remove updateregistry from if and reduces 1 if-else block

0xcuriousapple

lgtm, there is conflict but

SkandaBhat · 2022-01-11T09:19:16Z

@SamAg19 please fix conflicts.

SamAg19 added 2 commits December 20, 2021 16:15

Fix for collection deactivation bug

54545fe

tests added

d1bbf3f

SamAg19 changed the title ~~Penalty bug~~ Asset deactivation does not cause age penalty issue anymore Dec 23, 2021

SamAg19 added 2 commits December 23, 2021 17:48

Slither issues disabled

d55e641

Slither issues disabled

cf55104

SamAg19 commented Dec 23, 2021

View reviewed changes

0xcuriousapple reviewed Dec 24, 2021

View reviewed changes

SamAg19 added 3 commits January 3, 2022 14:41

redundant require lines replaced

04d2755

removed assetType and renamed asset manager to collection manager

89d8aa3

Merge remote-tracking branch 'upstream/master' into penaltyBug

c6c6ec6

SkandaBhat added the hold label Jan 7, 2022

indexToIdRegistry added

a3c1d26

SamAg19 requested review from 0xcuriousapple, hrishikeshio and SkandaBhat January 10, 2022 08:40

SamAg19 removed the hold label Jan 10, 2022

SamAg19 commented Jan 10, 2022

View reviewed changes

contracts/Core/CollectionManager.sol Outdated

}

function _updateRegistry() internal {

uint8 j = 1;

Copy link

Member Author

SamAg19 Jan 10, 2022

Choose a reason for hiding this comment

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

change it to uint16

SamAg19 commented Jan 10, 2022

View reviewed changes

SamAg19 added 3 commits January 10, 2022 17:38

changes fixed

508ef3e

slither fixes

558bf9e

Merge remote-tracking branch 'upstream/master' into penaltyBug

d2bec4f

0xcuriousapple reviewed Jan 11, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into penaltyBug

648fe12

0xcuriousapple approved these changes Jan 11, 2022

View reviewed changes

SkandaBhat approved these changes Jan 11, 2022

View reviewed changes

SkandaBhat merged commit dda1da8 into razor-network:master Jan 11, 2022

SamAg19 deleted the penaltyBug branch February 21, 2022 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset deactivation does not cause age penalty issue anymore #629

Asset deactivation does not cause age penalty issue anymore #629

SamAg19 commented Dec 23, 2021

coveralls commented Dec 23, 2021 •

edited

GasCheckBot commented Dec 23, 2021 •

edited

SamAg19 Dec 23, 2021

0xcuriousapple Dec 24, 2021

0xcuriousapple Dec 24, 2021

SamAg19 Jan 3, 2022

0xcuriousapple Dec 24, 2021

SamAg19 Jan 3, 2022

0xcuriousapple Dec 24, 2021

SamAg19 Jan 3, 2022

SamAg19 Jan 10, 2022

SamAg19 Jan 10, 2022

SamAg19 Jan 10, 2022

SamAg19 Jan 10, 2022

SamAg19 Jan 10, 2022

0xcuriousapple left a comment

SkandaBhat commented Jan 11, 2022

		collections[id].active = assetStatus;
		updateRegistry = epoch + 1;

Asset deactivation does not cause age penalty issue anymore #629

Asset deactivation does not cause age penalty issue anymore #629

Conversation

SamAg19 commented Dec 23, 2021

coveralls commented Dec 23, 2021 • edited

GasCheckBot commented Dec 23, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xcuriousapple left a comment

Choose a reason for hiding this comment

SkandaBhat commented Jan 11, 2022

coveralls commented Dec 23, 2021 •

edited

GasCheckBot commented Dec 23, 2021 •

edited