Upgrade modules and wallet contracts to solidity 0.6.10 #114

elenadimitrova · 2020-05-18T12:29:35Z

Upgrades contracts from /wallet and /modules to Solidity 0.6.10 while /infrastructure contracts remain on 0.5.4 except for the WalletFactory and TokenPriceProvider which are also upgraded to 0.6.10 (for that purpose I've had to move them out of /infrastructure to be compiled with the rest of the upgraded contracts until LimeChain/etherlime#325 is fixed.
Overlapping areas of logic were abstracted into interfaces with wider range compilation pragma i.e. pragma solidity >=0.5.4 <0.7.0; due to the fact there are no breaking changes to interfaces between the two major releases.

Includes refactoring that pulls apart the dependencies between storage contracts, BaseWallet and Module in order to allow storage contracts to remain on solc 0.5.4. Interactions between these layers is done via the IModule, IWallet, IGuardianStorage and ITransferStorage interfaces.

Also upgraded the openzeppelin-contracts library to 3.0.1

The following changes in gas consumption for benchmark tests is observed between releases:

Task [1 ETH = 500 USD]	Gas 0.5.4	Gas 0.6.9	Diff
Create a wallet without ENS (1 module)	486247	479970	-6277
Create a wallet without ENS (2 module)	515802	508848	-6954
Create a wallet without ENS (3 module)	545376	537746	-7630
Create a wallet without ENS (all modules)	832433	819941	-12492
Create a wallet with ENS (all modules)	832517	820025	-12492
Add first guardian (direct)	119451	119462	11
Request add second guardian (direct)	74359	73473	-886
Confirm add second guardian (direct)	92939	93452	513
Request revoke guardian (direct)	60275	59840	-435
Confirm revoke guardian (direct)	89758	89565	-193
Add a guardian (relayed)	154175	153712	-463
Revoke a guardian (relayed)	80909	80115	-794
Lock wallet (direct)	86299	86118	-181
Unlock wallet (direct)	51322	51028	-294
Lock wallet (relayed)	130832	130210	-622
Unlock wallet (relayed)	80867	80137	-730
Execute recovery	156766	155862	-904
Change limit (direct)	61931	61438	-493
Change limit (relayed)	97260	96337	-923
ETH transfer, limit disabled (direct)	57422	56340	-1082
ETH transfer, limit disabled (relayed)	93189	91689	-1500
ETH small transfer (direct)	66884	65748	-1136
ETH small transfer (relayed)	116025	114453	-1572
ETH transfer to whitelisted account (direct)	55073	54057	-1016
ETH transfer to whitelisted account (relayed)	90864	89401	-1463
ETH large transfer to untrusted account	80270	79116	-1154
ETH large transfer approval by one guardian	105440	104010	-1430
ETH large transfer approval by two guardians	118092	116390	-1702

Additionally we've removed the multiple inheritance in modules, e.g. contract NftTransfer is BaseModule, RelayerModule, OnlyOwnerModule -> contract NftTransfer is OnlyOwnerModule as the former causes the following compile errors caused by BaseModule, RelayerModule, OnlyOwnerModule all implementing versions of addModule and checkAndUpdateUniqueness methods.

/argent-contracts/contracts/modules/NftTransfer.sol:26:1: TypeError: Derived contract must override function "addModule". Two or more base classes define function with same name and parameter types.
contract NftTransfer is BaseModule, RelayerModule, OnlyOwnerModule {
^ (Relevant source part starts here and spans across multiple lines).
/Users/Elena/Source/argent-contracts/contracts/modules/common/BaseModule.sol:97:5: Definition in "BaseModule":
    function addModule(BaseWallet _wallet, Module _module) external virtual override strictOnlyWalletOwner(_wallet) {
    ^ (Relevant source part starts here and spans across multiple lines).
/Users/Elena/Source/argent-contracts/contracts/modules/common/OnlyOwnerModule.sol:48:5: Definition in "OnlyOwnerModule":
    function addModule(BaseWallet _wallet, Module _module) external override virtual onlyWalletOwner(_wallet) {
    ^ (Relevant source part starts here and spans across multiple lines).
,/Users/Elena/Source/argent-contracts/contracts/modules/NftTransfer.sol:26:1: TypeError: Derived contract must override function "checkAndUpdateUniqueness". Two or more base classes define function with same name and parameter types.
contract NftTransfer is BaseModule, RelayerModule, OnlyOwnerModule {
^ (Relevant source part starts here and spans across multiple lines).
/Users/Elena/Source/argent-contracts/contracts/modules/common/OnlyOwnerModule.sol:56:5: Definition in "OnlyOwnerModule":
    function checkAndUpdateUniqueness(BaseWallet _wallet, uint256 _nonce, bytes32 /* _signHash */) internal override returns (bool) {
    ^ (Relevant source part starts here and spans across multiple lines).
/Users/Elena/Source/argent-contracts/contracts/modules/common/RelayerModule.sol:157:5: Definition in "RelayerModule":
    function checkAndUpdateUniqueness(BaseWallet _wallet, uint256 _nonce, bytes32 _signHash) internal virtual returns (bool) {
    ^ (Relevant source part starts here and spans across multiple lines).

Note that due to crytic/slither#405 we've had to disable slither analysis on the wallet contracts. Logged this in trello to remind us to reenable it once slither supports analysis of the new 0.6. syntax.

olivdb · 2020-06-03T09:17:05Z

package.json

@@ -57,10 +61,10 @@
    "inquirer": "^7.0.0",
    "istanbul": "^0.4.5",
    "node-fetch": "^2.3.0",
-    "openzeppelin-solidity": "2.3.0",
+    "openzeppelin-solidity": "3.0.1",


Though both openzeppelin-solidity and @openzeppelin/contracts npm repositories are still being updated, @openzeppelin/contracts seems too be the preferred repo now (it's the one mentioned on their Github page).

I had no idea about the new one. Will update.

olivdb · 2020-06-03T09:21:49Z

contracts-test/LegacyBaseWallet.sol

@@ -24,7 +24,7 @@ pragma solidity ^0.5.4;

    /**
    * @dev Utility method to recover any ERC20 token that was sent to the
-    * module by mistake. 
+    * module by mistake.


Not related to this PR but this should say "contract" not "module"

olivdb · 2020-06-03T10:35:54Z

contracts-test/TestERC20.sol

-            _mint(_initialAccounts[i], _supply * 10**uint(_decimals));
+        super._setupDecimals(_decimals);
+        for (uint i = 0; i < _initialAccounts.length; i++) {
+            super._mint(_initialAccounts[i], _supply * 10**uint(_decimals));


Is super. required here? You didn't use it in TestERC721.mint()

olivdb · 2020-06-03T10:45:00Z

Perhaps we should prefix all contracts in contracts/legacy with LegacyXXX. Otherwise wouldn't the JSON artifacts in build/ produced by npm run compile:legacy clash with those produced by npm run compile:module ?

olivdb · 2020-06-03T11:26:15Z

contracts/wallet/Proxy.sol

        }
    }
+
+    receive() external payable {
+        emit Received(msg.value, msg.sender, msg.data);


Probably inconsequential but maybe worth notifying the clients that whereas before Received could not have been emitted with both msg.value == 0 and msg.data == 0, it is now possible.

I couldn't work out what's right for this event. Here it is emitted for 0 values while in the Proxy it was not.

Yes the previous implementation was confusing. I think it would have made more sense to check if (msg.data.length == 0) { emit ... } in the Proxy instead of if (msg.data.length == 0 && msg.value > 0) { emit ... }. The former is equivalent to your new implementation, which I think is good.

olivdb · 2020-06-03T11:36:34Z

contracts/wallet/BaseWallet.sol

            }
        }
    }
+
+    receive() external payable {
+        emit Received(msg.value, msg.sender, msg.data);


I think we can remove this event emission and leave receive() empty. If I'm not mistaken receive() in BaseWallet will only ever be called in our tests.

I've made Receive only emit on non-zero values 12c79ec

juniset · 2020-06-03T14:40:08Z

Shouldn't we move the /legacy folder out of /contracts (e.g. in /contracts-legacy)? I believe we should only have in /contracts what would be part of a security audit.

elenadimitrova · 2020-06-04T12:05:06Z

@olivdb @juniset regarding your comments on the legacy contracts, you're both right and I've pushed a change to both bring out the contracts into own folder at /contracts folder level as well as compile them in their own build-legacy folder to separate build artefacts we use in tests. That will allow us to preserve contract names kept for legacy purposes and avoid renaming of legacy contracts.

olivdb · 2020-06-04T15:02:13Z

I would remove the Legacy prefix from LegacyTransferManager and LegacyMakerManager. I would also remove the Legacy prefix from LegacyBaseWallet and move the file to contracts-legacy. But since there is a more recent "legacy" BaseWallet in there, I think we should place the contracts in contracts-legacy in subdirectories according to the last release in which they appeared. That would mean that all the legacy contracts would be placed under contract-legacy/v1.6.0 except the contract currently named LegacyBaseWallet.sol which should probably go in contract-legacy/v1.3.0

olivdb · 2020-06-11T11:05:22Z

contracts/modules/TransferManager.sol

@@ -130,7 +130,6 @@ contract TransferManager is OnlyOwnerModule, BaseTransfer, LimitManager {
        }
    }

-    // TODO: We should inherit the OnlyOwnerModule implementation


You probably tried the same things but I would have expected to be able to make TransferManager.addModule() call OnlyOwnerModule.addModule(_wallet, _module); instead of duplicating OnlyOwnerModule's code. Or alternatively, make OnlyOwnerModule be the last contract inherited by TransferManager (instead of LimitManager) and use super.addModule(_wallet, _module); in TransferManager.addModule(). But none of these solutions seem to work for me. Is that a Solidity bug or am I missing something?

Also, if we're going to duplicate OnlyOwnerModule's code, let's maybe change "BM: module is not registered" to "TM: module is not registered".

It's because addModule is external, see Chris's explanation in this discussion ethereum/solidity#3881 (comment)
We have the option of removing addModule from the IModule interface, make it public and then use super. Atm it's not used off that interface so I could go down that route. What do you think?

I personally think it is important to keep the addModule method in the IModule interface as it will protect us from future mistake.

Guaranteeing that every module can add another module combined with the fact that a wallet needs at least one module ensures the wallet can never be in a blocked state (in the sense of not being upgradable).

Yet another way is to switch IModule from an interface to abstract contract which allows public functions with no implementation at the same time as mandating all functions to be implemented.

Yes that works for me.

We could keep IModule as-is but make addModule in BaseModule and OnlyOwnerModule be public instead of external.

That's also an option. I don't feel strongly for either of the two. I'll just go with your version as it causes less changes to current state. Let me know if that's okay

olivdb · 2020-06-11T11:20:57Z

contracts/wallet/BaseWallet.sol

            }
        }
    }
+
+    receive() external payable {


This function is not needed and can be removed entirely. It will never be reached from the Proxy (since calls to the Proxy with msg.data == 0 are not forwarded to the BaseWallet).

It's actually used in tests as we're not creating wallets that are Proxies but just new-ing up a BaseWallet instance. It's arguable whether this is correct and we shouldn't be testing with a proper Proxy-based wallet.

It helps fix the coverage problem though as I can remove the logic of receive which allows coverage to pass.

Removing that method won't break the tests as they currently stand. The fallback() of BaseWallet will be executed instead, so I believe we can remove receive() regardless of whether we decide to rewrite the tests to use Proxy or not

I does break the tests as fallback won't execute when there's no calldata received. That was the whole point of splitting the fallback function.

As per the doc:

A payable fallback function is also executed for plain Ether transfers, if no receive Ether function is present. [...]

https://solidity.readthedocs.io/en/v0.6.10/contracts.html#fallback-function

I also verified in Remix that the following contract is able to receive ether via empty calldata:

pragma solidity ^0.6.10; contract FallbackTest { fallback() external payable {} }

Okay it's not failing because plain ether is not accepted but because the call to fallback costs more than 21000 gas. https://app.circleci.com/pipelines/github/argentlabs/argent-contracts/707/workflows/26010b94-04c4-4a0c-ad62-c9cbe0e39afe/jobs/897
I can't think of any way to completely remove the receive function off BaseWallet really but feel free to push any changes you think are possible.

I see. I don't love the idea of having a receive() function in BaseWallet that is only there for tests. On the other hand having to use a Proxy + BaseWallet in all our tests feels a bit heavy. Maybe using a BaseWalletTest in our tests, that inherits from BaseWallet and only adds an empty receive() to BaseWallet could be a middle ground? If not, let's just keep the empty receive().

Thinking about this more, testing with a Proxy + BaseWallet is probably the right move. We could maybe add a createWallet() method to test-manager.js that deploys both contracts + wraps the proxy with the BaseWallet abi.

juniset · 2020-06-11T14:36:05Z

The deployment script 5_deploy_modules.js does not use the newly deployed GuardianStorage and TransferStorage contracts to instantiate some of the modules.

…tion

This helps solve sc-forks/solidity-coverage#516

and instead move all logic to standalone FakeWallet contract

olivdb · 2020-06-15T11:45:03Z

Why doesn't contracts-legacy/v1.3.0 contain only BaseWallet.sol and OldTestModule.sol? All the other files in there seem identical to those in contracts-legacy/v1.5.0 and are not used in tests.

olivdb · 2020-06-15T11:56:18Z

I think contracts-legacy/v1.5.0 should be renamed contracts-legacy/v1.6.0 as all the contracts in there are in their versions last used in 1.6.

elenadimitrova · 2020-06-15T12:42:54Z

Why doesn't contracts-legacy/v1.3.0 contain only BaseWallet.sol and OldTestModule.sol? All the other files in there seem identical to those in contracts-legacy/v1.5.0 and are not used in tests.

Because otherwise we'll have cross references between legacy folders, in this case 1.3.0 importing contracts from 1.5.0. I think we should keep legacy copies atomic to a given release, otherwise it will likely get very tangled up.

elenadimitrova · 2020-06-15T15:26:11Z

I think contracts-legacy/v1.5.0 should be renamed contracts-legacy/v1.6.0 as all the contracts in there are in their versions last used in 1.6.

TransferManager is from 1.5 but I should probably change it to the 1.6 version as we are supporting up to one version back in upgrades. MakerManager hasn't changed so I can rename the folder and it'll be correct for all.

elenadimitrova self-assigned this May 18, 2020

elenadimitrova mentioned this pull request May 18, 2020

Remove circular dependencies between BaseModule and BaseWallet #113

Closed

elenadimitrova force-pushed the maintenance/upgrade-solc-0.6.0 branch 2 times, most recently from d1fed32 to 4bfc474 Compare May 21, 2020 07:14

elenadimitrova force-pushed the maintenance/upgrade-solc-0.6.0 branch 4 times, most recently from 4ecbf58 to 5c4cf40 Compare June 1, 2020 08:51

olivdb reviewed Jun 3, 2020

View reviewed changes

elenadimitrova force-pushed the maintenance/upgrade-solc-0.6.0 branch from 9994690 to 1a4273d Compare June 4, 2020 12:18

elenadimitrova force-pushed the maintenance/upgrade-solc-0.6.0 branch 3 times, most recently from 6a65656 to 4a263b2 Compare June 10, 2020 11:39

elenadimitrova marked this pull request as ready for review June 10, 2020 12:28

elenadimitrova changed the title ~~Upgrade modules and wallet contracts to solidity 0.6.8~~ Upgrade modules and wallet contracts to solidity 0.6.9 Jun 10, 2020

elenadimitrova force-pushed the maintenance/upgrade-solc-0.6.0 branch 2 times, most recently from 13cb544 to 4eb4581 Compare June 11, 2020 07:31

olivdb reviewed Jun 11, 2020

View reviewed changes

elenadimitrova force-pushed the maintenance/upgrade-solc-0.6.0 branch 2 times, most recently from c3cd35f to c03e3cc Compare June 12, 2020 09:20

elenadimitrova added 18 commits June 15, 2020 13:17

Use 0.6 syntax for create2 calls

6e0a37b

Update ganache-cli version

faeaaed

Make etherlime coverage work with more complex compilation setup

6657a18

Remove fixed gasLimit in transactions

56c4c5b

Fix slither run

40bfa7a

Log Received event for non zero msg transfers only

01aa8fa

Adjust coverage thresholds for the new contracts folder structure

6537489

Remove todo in code

8d389f2

Switch to latest solidity 0.6.9

815b2b7

Upgrade etherlime-argent

07514f3

Fix storage contracts references in modules deployment

1cf397f

Make addModule function public and override external interface defini…

d4cda69

…tion

Remove event from BaseWallet receive function

8d56f4d

This helps solve sc-forks/solidity-coverage#516

Reshuffle legacy contracts to dedicated release folders

77fe198

Emit Received event in Proxy on 0 value transfers

7dd2952

Switch to solc 0.6.10

eb486ae

Stop overriding BaseWallet.invoke to make it non-virtual

92c4209

and instead move all logic to standalone FakeWallet contract

Use Proxy when testing Wallet logic

65e4074

elenadimitrova force-pushed the maintenance/upgrade-solc-0.6.0 branch from 3e9b301 to 65e4074 Compare June 15, 2020 10:24

juniset approved these changes Jun 15, 2020

View reviewed changes

elenadimitrova added 2 commits June 15, 2020 22:09

Rename 1.5 to 1.6 to match respective release number

a585e98

Replace legacy transfer manager for tests with one version earlier - 1.6

1cd7ef8

olivdb approved these changes Jun 16, 2020

View reviewed changes

elenadimitrova merged commit c937a4d into develop Jun 16, 2020

elenadimitrova deleted the maintenance/upgrade-solc-0.6.0 branch June 16, 2020 08:20

elenadimitrova mentioned this pull request Jul 16, 2020

Switch Solidity linter to solhint #137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade modules and wallet contracts to solidity 0.6.10 #114

Upgrade modules and wallet contracts to solidity 0.6.10 #114

elenadimitrova commented May 18, 2020 •

edited

Loading

olivdb Jun 3, 2020

elenadimitrova Jun 3, 2020

olivdb Jun 3, 2020

olivdb Jun 3, 2020

olivdb commented Jun 3, 2020

olivdb Jun 3, 2020

elenadimitrova Jun 3, 2020

olivdb Jun 3, 2020 •

edited

Loading

olivdb Jun 3, 2020 •

edited

Loading

elenadimitrova Jun 10, 2020

juniset commented Jun 3, 2020 •

edited

Loading

elenadimitrova commented Jun 4, 2020 •

edited

Loading

olivdb commented Jun 4, 2020 •

edited

Loading

olivdb Jun 11, 2020 •

edited

Loading

elenadimitrova Jun 12, 2020

juniset Jun 12, 2020

elenadimitrova Jun 12, 2020

juniset Jun 12, 2020

olivdb Jun 12, 2020

elenadimitrova Jun 12, 2020

olivdb Jun 11, 2020 •

edited

Loading

elenadimitrova Jun 12, 2020

elenadimitrova Jun 12, 2020

olivdb Jun 12, 2020 •

edited

Loading

elenadimitrova Jun 12, 2020

olivdb Jun 12, 2020

elenadimitrova Jun 12, 2020

olivdb Jun 12, 2020

olivdb Jun 12, 2020 •

edited

Loading

juniset commented Jun 11, 2020 •

edited

Loading

olivdb commented Jun 15, 2020 •

edited

Loading

olivdb commented Jun 15, 2020

elenadimitrova commented Jun 15, 2020

elenadimitrova commented Jun 15, 2020 •

edited

Loading

Upgrade modules and wallet contracts to solidity 0.6.10 #114

Upgrade modules and wallet contracts to solidity 0.6.10 #114

Conversation

elenadimitrova commented May 18, 2020 • edited Loading

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

olivdb commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivdb Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

olivdb Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juniset commented Jun 3, 2020 • edited Loading

elenadimitrova commented Jun 4, 2020 • edited Loading

olivdb commented Jun 4, 2020 • edited Loading

olivdb Jun 11, 2020 • edited Loading

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

olivdb Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivdb Jun 12, 2020 • edited Loading

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

olivdb Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

juniset commented Jun 11, 2020 • edited Loading

olivdb commented Jun 15, 2020 • edited Loading

olivdb commented Jun 15, 2020

elenadimitrova commented Jun 15, 2020

elenadimitrova commented Jun 15, 2020 • edited Loading

elenadimitrova commented May 18, 2020 •

edited

Loading

olivdb Jun 3, 2020 •

edited

Loading

olivdb Jun 3, 2020 •

edited

Loading

juniset commented Jun 3, 2020 •

edited

Loading

elenadimitrova commented Jun 4, 2020 •

edited

Loading

olivdb commented Jun 4, 2020 •

edited

Loading

olivdb Jun 11, 2020 •

edited

Loading

olivdb Jun 11, 2020 •

edited

Loading

olivdb Jun 12, 2020 •

edited

Loading

olivdb Jun 12, 2020 •

edited

Loading

juniset commented Jun 11, 2020 •

edited

Loading

olivdb commented Jun 15, 2020 •

edited

Loading

elenadimitrova commented Jun 15, 2020 •

edited

Loading