search

polymorpher / sms-wallet

SMS Wallet - non-custodial wallet bound to phone number
https://smswallet.xyz
6 stars 4 forks source link

AssetManager Contract #1

Closed johnwhitton closed 2 years ago

johnwhitton commented 2 years ago

This first PR is at a logical point to be Merged the Todo items and clarifications have been documented in Issue https://github.com/polymorpher/sms-wallet/issues/3 and can be worked on separately.

ToDo

Outstanding Issues/Clarifications

Done

examples

  1. If they have deposited 5 Native Tokens and authorized 2 Tokens then withdraw 4 tokens then we set authorized 1.
  2. If they have deposited 5 Native Tokens and authorized 3 Tokens then withdraw 1 token currently we leave the authorization at 3 Tokens (rather than reducing it to 2 tokens). 
    
**Decision: withdrawal logic does not effect authorizations (logic above removed)**
- [x] AssetManager comments
- [x] Review [Reentrancy guard](https://docs.openzeppelin.com/contracts/4.x/api/security#ReentrancyGuard) **not needed**
- [x] review [PullPayment]( https://docs.openzeppelin.com/contracts/4.x/api/security#PullPayment) **not needed**

Clarifications also added in the SMS Wallet Design Document

polymorpher commented 2 years ago

We can remove all boilerplates and auto-generated files. This includes docs/, elin/, scripts/, data/abi. Prettier is inconsistent with eslint - I suggest we remove prettier for now. README and some other files seem to be boilerplates - can they be removed until needed, or minimized to essential elements only?

polymorpher commented 2 years ago

I think we will need multiple operators to allow the server serving multiple SMS commands at the same time. This is similar to the problem with relayer in https://github.com/polymorpher/one-wallet . We could consider using https://docs.openzeppelin.com/contracts/2.x/access-control#role-based-access-control , or just to keep it simple and expand operator into an array

polymorpher commented 2 years ago

All looks good. Aside from minor things commented above, the token transfer tests are missing some balance checks and event emission checks. I will re-run some tests when you add them

polymorpher commented 2 years ago

I have these test failures:

  1) AssetManager
       checkTransfer
         Positive transfer test:

      AssertionError: Expected "0" to be equal 1000000000000000000
      + expected - actual

       {
      -  "_hex": "0x0de0b6b3a7640000"
      +  "_hex": "0x00"
         "_isBigNumber": true
       }

      at Context.<anonymous> (test/AssetManager.ts:265:12)

  2) AssetManager
       checkAssetManager
         Positive walk-through, deposit, withdraw, approve, send:

      AssertionError: Expected "0" to be equal 1000000000000000000
      + expected - actual

       {
      -  "_hex": "0x0de0b6b3a7640000"
      +  "_hex": "0x00"
         "_isBigNumber": true
       }

After they are fixed I can merge the PR

polymorpher commented 2 years ago

I will also make a note here that later we should probably also remove yarn.lock from all folders in the repository per discussion here https://github.com/yarnpkg/yarn/issues/1583 . I am also concerned that specific versions pointed in yarn.lock may unknowingly carry malware (as recent large scale exploits and hacks have shown). So far packages we used and installed seem to be safe. I checked each package and their dependencies one by one

johnwhitton commented 2 years ago

Additional note After merging changes need to run yarn clean to avoid errors below Moving forward we should look at whether to include build related files such and deploy related files such as the .openzepplin folder currently we are additionally logging contract addresses in the deploy scripts.

git pull
yarn test
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat test
Generating typings for: 36 artifacts in dir: typechain for target: ethers-v5
Successfully generated 57 typings!
Compiled 33 Solidity files successfully
An unexpected error occurred:

[Error: ENOENT: no such file or directory, open '/Users/john/one-wallet/sms-wallet/chaincode/build/build-info/a99f4ae7a21a1354b2beba87da171827.json'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/john/one-wallet/sms-wallet/chaincode/build/build-info/a99f4ae7a21a1354b2beba87da171827.json'
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
johnwhitton commented 2 years ago

When running yarn coverage which uses solidity-coverage we get

Compilation:
============

Warning: Contract code size exceeds 24576 bytes (a limit introduced in Spurious Dragon). This contract may not be deployable on mainnet. Consider enabling the optimizer (with a low "runs" value!), turning off revert strings, or using libraries.
  --> contracts/AssetManager.sol:28:1:
   |
28 | contract AssetManager is Initializable, PausableUpgradeable, AccessControlEnumerableUpgradeable {
   | ^ (Relevant source part starts here and spans across multiple lines).

Investigating workarounds

  1. We already have enabled optimizer in hardhat.config.ts
  2. We could change all require statements to if test and revert using errors.
  3. We could use diamonds see here and here

I added in hardhat-contract-sizer And when running yarn compile with optimizer enabled it shows

$ yarn compile
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat compile
Generating typings for: 36 artifacts in dir: typechain for target: ethers-v5
Successfully generated 57 typings!
Compiled 33 Solidity files successfully
✅ Generated documentation for 36 contracts
 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  AssetManager   ·       9.131  ·                │
 ·-----------------|--------------|----------------·
✨  Done in 12.54s.

without optimization it shows

 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  AssetManager   ·      17.063  ·        +7.932  │
 ·-----------------|--------------|----------------·

And yarn deploy with optimizer enabled produces

yarn deploy
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat deploy
Nothing to compile
No need to generate any newer typings.
✅ Generated documentation for 36 contracts
 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  AssetManager   ·       9.131  ·                │
 ·-----------------|--------------|----------------·
operators: ["0x70997970C51812dc3A010C7d01b50e0d17dc79C8","0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC","0x90F79bf6EB2c4f870365E785982E1f101E93b906"]
AssetManager deployed to: 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
AssetManager Operator Threshold: 10
operatorCount : 3
Operator [0]: 0x70997970C51812dc3A010C7d01b50e0d17dc79C8
Operator [1]: 0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC
Operator [2]: 0x90F79bf6EB2c4f870365E785982E1f101E93b906
AssetManager Global User Auth Limit: 1000000.0
AssetManager Global User Auth Limit: 100000.0
✨  Done in 7.83s.

Will leave optimizer enabled Believe that this is just an issue with coverage and that when deploying AssetManager will have a size of 9131 bytes which is less than the 24576 bytes limit.

If we have issues with deploy then we can revisit.

polymorpher commented 2 years ago

Is there some flexibilities in the parameters for deploying proxy-upgradeable contracts? Rather than having OpenZepplin to manage everything and hide all the details in obscure local JSON files, ideally we want to keep the address of the proxy, the logic contract's address, and the storage contract's address in some config or environment files, and just load them at times of management / deployment.

Additional note After merging changes need to run yarn clean to avoid errors below Moving forward we should look at whether to include build related files such and deploy related files such as the .openzepplin folder currently we are additionally logging contract addresses in the deploy scripts.

git pull
yarn test
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat test
Generating typings for: 36 artifacts in dir: typechain for target: ethers-v5
Successfully generated 57 typings!
Compiled 33 Solidity files successfully
An unexpected error occurred:

[Error: ENOENT: no such file or directory, open '/Users/john/one-wallet/sms-wallet/chaincode/build/build-info/a99f4ae7a21a1354b2beba87da171827.json'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/john/one-wallet/sms-wallet/chaincode/build/build-info/a99f4ae7a21a1354b2beba87da171827.json'
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
polymorpher commented 2 years ago

Optimizers may get rid of the extra storage slots allocated by the upgradeable contracts (gap), which are reserved for upgrading logic contracts. I noticed each inherited upgradeable made their own allocation. It is probably not efficient and we may need to allocate our own gap. Can you check OpenZepplin documentations and developers discussions and see if is some consensus on the recommended way of resolving these issues?