Renamed SafeCore to Anchor #549

gulshanvasnani · 2018-12-14T13:38:23Z

PR does following :-

Renames SafeCore contract name to Anchor.
Updates the references of SafeCore in unit test cases.
Changes the method name commitStateRoot to anchorStateRoot in Anchor contract.
Changes the method name setCoCoreAddress to setCoAnchorAddress in Anchor contract.

Fixes: #537

schemar

Nice one 🏎💨

Just some comments on wording and indentation 👍

schemar · 2018-12-14T13:41:13Z

contracts/gateway/Anchor.sol

- * @notice SafeCore stores another chain's state roots. It stores the address of
- *         the co-core, which will be the safe core on the other chain. State
+ * @notice Anchor stores another chain's state roots. It stores the address of
+ *         the co-core, which will be the anchor on the other chain. State


Suggested change

* the co-core, which will be the anchor on the other chain. State

* the co-anchor, which will be the anchor on the other chain. State

schemar · 2018-12-14T13:42:39Z

test/gateway/anchor/get_latest_state_root_block_height.js

@@ -41,8 +41,8 @@ contract('SafeCore.getLatestStateRootBlockHeight()', function (accounts) {
    blockHeight = new BN(5);
    stateRoot = web3.utils.sha3("dummy_state_root");
    membersManager = await MockMembersManager.new(owner, worker);
-
-    safeCore = await SafeCore.new(
+


Suggested change

schemar · 2018-12-14T13:42:54Z

test/gateway/anchor/set_co_core_address.js

@@ -46,8 +46,8 @@ contract('SafeCore.setCoCoreAddress()', function (accounts) {
    stateRoot = web3.utils.sha3("dummy_state_root");
    membersManager = await MockMembersManager.new(owner, worker);
    coCoreAddress = accounts[6];
-
-    safeCore = await SafeCore.new(
+


Suggested change

schemar · 2018-12-14T13:43:12Z

test/gateway/anchor/set_co_core_address.js

@@ -61,7 +61,7 @@ contract('SafeCore.setCoCoreAddress()', function (accounts) {
    coCoreAddress = NullAddress;

    await Utils.expectRevert(
-      safeCore.setCoCoreAddress(coCoreAddress, { from: owner }),
+        anchor.setCoCoreAddress(coCoreAddress, { from: owner }),


In many places:

Suggested change

anchor.setCoCoreAddress(coCoreAddress, { from: owner }),

anchor.setCoCoreAddress(coCoreAddress, { from: owner }),

schemar · 2018-12-14T13:43:53Z

contracts/gateway/Anchor.sol

- * @notice SafeCore stores another chain's state roots. It stores the address of
- *         the co-core, which will be the safe core on the other chain. State
+ * @notice Anchor stores another chain's state roots. It stores the address of
+ *         the co-core, which will be the anchor on the other chain. State
 *         roots are exchanged bidirectionally between the core and the co-core


Suggested change

* roots are exchanged bidirectionally between the core and the co-core

* roots are exchanged bidirectionally between the anchor and the co-anchor

contracts/gateway/Anchor.sol

…,modified unit test cases and removed reference of safe core

schemar

LGTM

But please wait for at least one other review before merging.

0xsarvesh

Almost done. 🙌
Some minor fixes are required.

Committed should also be changed to Anchoredin various files.

Also fix terminology in below contracts:

StateRootInterface
GatewayLib
GatewayBase
MockAnchor L18, L42

I am not sure if StateRootInterface _core, should be call as StateRootInterface _anchor.

0xsarvesh · 2018-12-18T04:32:07Z

contracts/gateway/Anchor.sol

@@ -184,7 +184,7 @@ contract SafeCore is StateRootInterface, Organized {
        // Input block height should be valid


Please add . in the comments.

0xsarvesh · 2018-12-18T04:33:21Z

contracts/test/core/MockAnchor.sol


    /*  Public functions */

    /**
-     * @notice Contract constructor
+     * @notice Contract constructor.
     *
     *  @param _remoteChainId _remoteChainId is the chain id of the chain that
     *                        this core tracks.


Suggested change

* this core tracks.

* this anchor tracks.

contracts/test/core/MockAnchor.sol

0xsarvesh · 2018-12-18T04:35:50Z

contracts/test/gateway/MockGatewayBase.sol

@@ -42,7 +42,7 @@ contract MockGatewayBase is GatewayBase {
     *
     *  @dev proveGateway can be called by anyone to verify merkle proof of
     *       gateway/co-gateway contract address. Trust factor is brought by
-     *       stateRoots mapping. stateRoot is committed in commitStateRoot
+     *       stateRoots mapping. stateRoot is committed in anchorStateRoot


Suggested change

* stateRoots mapping. stateRoot is committed in anchorStateRoot

* stateRoots mapping. stateRoot is anchored in anchorStateRoot

0xsarvesh · 2018-12-18T04:38:12Z

test/gateway/anchor/get_latest_state_root_block_height.js

+
+    });
+
+    it('should return the latest committed state root block height', async () => {


Suggested change

it('should return the latest committed state root block height', async () => {

it('should return the latest anchored state root block height', async () => {

0xsarvesh · 2018-12-18T04:38:50Z

test/gateway/anchor/get_state_root.js

+
+    });
+
+    it('should return the latest committed state root', async () => {


Suggested change

it('should return the latest committed state root', async () => {

it('should return the latest anchored state root', async () => {

…m/gulshanvasnani/mosaic-contracts into gulshan/gh_537_safecore_to_anchor

…afecore_to_anchor # Conflicts: # contracts/gateway/Anchor.sol # contracts/test/core/MockAnchor.sol # test/gateway/gateway_base/prove_gateway.js # test/gateway/safe_core/commit_state_root.js # test/gateway/safe_core/constructor.js # test/gateway/safe_core/get_latest_state_root_block_height.js # test/gateway/safe_core/get_remote_chainId.js # test/gateway/safe_core/get_state_root.js # test/gateway/safe_core/set_co_core_address.js

schemar · 2018-12-18T13:10:04Z

Also fix terminology in below contracts:

StateRootInterface

GatewayLib

GatewayBase

MockAnchor L18, L42

I don't think the gateway code should be aware whether it's an anchor or a core. The gateways should only care about the state root interface. E.g. "state root provider". Or am I missing something?

@gulshanvasnani @sarvesh-ost

0xsarvesh · 2018-12-18T14:21:10Z

Also fix terminology in below contracts:

StateRootInterface

GatewayLib

GatewayBase

MockAnchor L18, L42

I don't think the gateway code should be aware whether it's an anchor or a core. The gateways should only care about the state root interface. E.g. "state root provider". Or am I missing something?

What should be the name of the variable which represents contract instance implementing StateRootInterface?
Currently, it's core which seems incorrect, as Anchor contract is providing state roots.
Do we need third terminology other than core and Anchor? 🤔

schemar · 2018-12-18T14:58:47Z

Also fix terminology in below contracts:

StateRootInterface

GatewayLib

GatewayBase

MockAnchor L18, L42

I don't think the gateway code should be aware whether it's an anchor or a core. The gateways should only care about the state root interface. E.g. "state root provider". Or am I missing something?

What should be the name of the variable which represents contract instance implementing StateRootInterface?
Currently, it's core which seems incorrect, as Anchor contract is providing state roots.
Do we need third terminology other than core and Anchor? 🤔

Yes, we do. As it can be both. It can be an anchor as well as a core. In fact, it can be anything that implements the state root interface. It's just that in the past only different kinds of cores implemented that interface. Now that the safe core is called anchor, we need a more general name for the variable of that interface type.

…afecore_to_anchor

…m/gulshanvasnani/mosaic-contracts into gulshan/gh_537_safecore_to_anchor

gulshanvasnani · 2018-12-19T07:03:33Z

Also fix terminology in below contracts:

StateRootInterface

GatewayLib

GatewayBase

MockAnchor L18, L42

I don't think the gateway code should be aware whether it's an anchor or a core. The gateways should only care about the state root interface. E.g. "state root provider". Or am I missing something?

What should be the name of the variable which represents contract instance implementing StateRootInterface?
Currently, it's core which seems incorrect, as Anchor contract is providing state roots.
Do we need third terminology other than core and Anchor? 🤔

Yes, we do. As it can be both. It can be an anchor as well as a core. In fact, it can be anything that implements the state root interface. It's just that in the past only different kinds of cores implemented that interface. Now that the safe core is called anchor, we need a more general name for the variable of that interface type.

Name can be stateRoots.

…t-cases

…afecore_to_anchor

…m/gulshanvasnani/mosaic-contracts into gulshan/gh_537_safecore_to_anchor

schemar

Hey @gulshanvasnani,
this looks already much better, but unfortunately you went over the top when it comes to renaming things...

All the contracts must be checked again, as there are many places where it is now called anchor, bit should be more general.
All the tests must be checked again, as there are many places where it is now called anchor, but should be more general.
I added a relevant comment to some of the affected files.

See also some other minor comments in-line.

contracts/StateRootInterface.sol

contracts/gateway/EIP20CoGateway.sol

contracts/gateway/EIP20Gateway.sol

contracts/gateway/GatewayBase.sol

test/gateway/anchor/anchor_state_root.js

test/gateway/eip20_cogateway/constructor.js

test/gateway/eip20_cogateway/progress_mint.js

test/gateway/eip20_gateway/activate_gateway.js

test/gateway/eip20_gateway/constructor.js

… root interface

…erface variable in constructor and corrected js styling

…m/gulshanvasnani/mosaic-contracts into gulshan/gh_537_safecore_to_anchor

schemar

Almost there 🧗🏽‍♂️

contracts/gateway/GatewayBase.sol

contracts/test/gateway/MockGatewayBase.sol

test/gateway/anchor/anchor_state_root.js

test/gateway/eip20_cogateway/redeem.js

0xsarvesh

LGTM ⚓️👍🚜

schemar

Awesome

test/gateway/anchor/anchor_state_root.js

gulshanvasnani added 2 commits December 14, 2018 18:58

Renamed SafeCore contract and references of safecore to anchor

24119cd

Modified the documentation

71a8f81

benjaminbollen added the review label Dec 14, 2018

schemar requested review from 0xsarvesh and deepesh-kn December 14, 2018 13:39

schemar assigned gulshanvasnani Dec 14, 2018

schemar suggested changes Dec 14, 2018

View reviewed changes

gulshanvasnani and others added 2 commits December 16, 2018 00:05

Changed the name of the setCoCoreAddress method to setCoAnchorAddress…

4116f54

…,modified unit test cases and removed reference of safe core

Merge branch 'develop' into gulshan/gh_537_safecore_to_anchor

f49e0df

schemar approved these changes Dec 17, 2018

View reviewed changes

0xsarvesh suggested changes Dec 18, 2018

View reviewed changes

gulshanvasnani added 5 commits December 18, 2018 12:11

Changes done as per PR feedback

cdf2b22

Merge branch 'gulshan/gh_537_safecore_to_anchor' of https://github.co…

eb7934a

…m/gulshanvasnani/mosaic-contracts into gulshan/gh_537_safecore_to_anchor

Changed all references of core in the constructor

e66da54

Changed all references of core in the constructor

8bf24fb

Merge branch 'develop' into gulshan/gh_537_safecore_to_anchor

115d5a9

gulshanvasnani added 2 commits December 19, 2018 11:22

Merge remote-tracking branch 'upstream/develop' into gulshan/gh_537_s…

659132c

…afecore_to_anchor

Merge branch 'gulshan/gh_537_safecore_to_anchor' of https://github.co…

feee710

…m/gulshanvasnani/mosaic-contracts into gulshan/gh_537_safecore_to_anchor

gulshanvasnani closed this Dec 19, 2018

benjaminbollen removed the review label Dec 19, 2018

gulshanvasnani reopened this Dec 19, 2018

benjaminbollen added the review label Dec 19, 2018

gulshanvasnani and others added 3 commits December 19, 2018 13:14

Fixed styling

ca4e4ae

Added maxNumberStateRoots in the constructor and updated the unit tes…

e5377c4

…t-cases

Merge branch 'develop' into gulshan/gh_537_safecore_to_anchor

8956788

gulshanvasnani added 4 commits December 19, 2018 16:41

Merge remote-tracking branch 'upstream/develop' into gulshan/gh_537_s…

469088c

…afecore_to_anchor

Deleting safe core unit test-cases

667c5fd

Renamed of variable to mockAnchor from mockSafeCore

7626721

Merge branch 'gulshan/gh_537_safecore_to_anchor' of https://github.co…

6075fcc

…m/gulshanvasnani/mosaic-contracts into gulshan/gh_537_safecore_to_anchor

schemar suggested changes Dec 19, 2018

View reviewed changes

gulshanvasnani and others added 5 commits December 19, 2018 21:32

Changes done for introducing generic name for variable name for state…

b7cbd2d

… root interface

Merge branch 'develop' into gulshan/gh_537_safecore_to_anchor

8c04438

Changes done to update unit test cases,documentation for stateRootInt…

630bcf6

…erface variable in constructor and corrected js styling

Merge branch 'gulshan/gh_537_safecore_to_anchor' of https://github.co…

a4db386

…m/gulshanvasnani/mosaic-contracts into gulshan/gh_537_safecore_to_anchor

Changes done to update unit test cases messages and corrected js styling

34030e3

schemar suggested changes Dec 20, 2018

View reviewed changes

contracts/gateway/GatewayBase.sol Outdated Show resolved Hide resolved

contracts/test/gateway/MockGatewayBase.sol Outdated Show resolved Hide resolved

test/gateway/anchor/anchor_state_root.js Show resolved Hide resolved

test/gateway/eip20_cogateway/redeem.js Outdated Show resolved Hide resolved

Code cleanup and updated documentation as suggested in PR feedback

8735258

gulshanvasnani force-pushed the gulshan/gh_537_safecore_to_anchor branch from 9300879 to 8735258 Compare December 20, 2018 13:49

gulshanvasnani mentioned this pull request Dec 21, 2018

Only organization should anchor state roots #560

Merged

Added license in the contract and updated documentation

aff1bde

0xsarvesh approved these changes Dec 21, 2018

View reviewed changes

schemar approved these changes Dec 21, 2018

View reviewed changes

test/gateway/anchor/anchor_state_root.js Show resolved Hide resolved

schemar merged commit 287d68f into OpenST:develop Dec 21, 2018

benjaminbollen removed the review label Dec 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renamed SafeCore to Anchor #549

Renamed SafeCore to Anchor #549

gulshanvasnani commented Dec 14, 2018 •

edited

Loading

schemar left a comment

schemar Dec 14, 2018

schemar Dec 14, 2018

schemar Dec 14, 2018

schemar Dec 14, 2018

schemar Dec 14, 2018

schemar left a comment

0xsarvesh left a comment

0xsarvesh Dec 18, 2018

0xsarvesh Dec 18, 2018

0xsarvesh Dec 18, 2018

0xsarvesh Dec 18, 2018

0xsarvesh Dec 18, 2018

schemar commented Dec 18, 2018 •

edited

Loading

0xsarvesh commented Dec 18, 2018 •

edited

Loading

schemar commented Dec 18, 2018

gulshanvasnani commented Dec 19, 2018 •

edited

Loading

schemar left a comment

schemar left a comment

0xsarvesh left a comment

schemar left a comment

	* the co-core, which will be the anchor on the other chain. State
	* the co-anchor, which will be the anchor on the other chain. State

	anchor.setCoCoreAddress(coCoreAddress, { from: owner }),
	anchor.setCoCoreAddress(coCoreAddress, { from: owner }),

	* roots are exchanged bidirectionally between the core and the co-core
	* roots are exchanged bidirectionally between the anchor and the co-anchor

		@@ -184,7 +184,7 @@ contract SafeCore is StateRootInterface, Organized {
		// Input block height should be valid

	* stateRoots mapping. stateRoot is committed in anchorStateRoot
	* stateRoots mapping. stateRoot is anchored in anchorStateRoot


		});

		it('should return the latest committed state root block height', async () => {

	it('should return the latest committed state root block height', async () => {
	it('should return the latest anchored state root block height', async () => {

Renamed SafeCore to Anchor #549

Renamed SafeCore to Anchor #549

Conversation

gulshanvasnani commented Dec 14, 2018 • edited Loading

schemar left a 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

schemar left a comment

Choose a reason for hiding this comment

0xsarvesh left a 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

schemar commented Dec 18, 2018 • edited Loading

0xsarvesh commented Dec 18, 2018 • edited Loading

schemar commented Dec 18, 2018

gulshanvasnani commented Dec 19, 2018 • edited Loading

schemar left a comment

Choose a reason for hiding this comment

schemar left a comment

Choose a reason for hiding this comment

0xsarvesh left a comment

Choose a reason for hiding this comment

schemar left a comment

Choose a reason for hiding this comment

gulshanvasnani commented Dec 14, 2018 •

edited

Loading

schemar commented Dec 18, 2018 •

edited

Loading

0xsarvesh commented Dec 18, 2018 •

edited

Loading

gulshanvasnani commented Dec 19, 2018 •

edited

Loading