MiniWallet Smart Contracts Testing and Deployment

[johnwhitton]

This Issue tracks the next steps for the SMS-Wallet Smart Contracts (Chaincode) It follows on from Pull Request https://github.com/polymorpher/sms-wallet/pull/1 enhancing and hardening testing, deployment and adding in additional ERC777 functionality

ToDo

Priority Tasks

• [x] Rename AssetManager.sol to MiniWallet.sol
• [x] Add deposit functionality to approve
• [ ] Change all requires to if tests and error messages
• [ ] Testing
  • [ ] Additional tests for require, modifiers and pausable functions
  • [ ] Additional tests when adding operators over thresholds
  • [ ] Additional tests for depositing or approving funds over global limits
  • [ ] Refactor testing
• [x] Deployment
  • [x] Review deployment best practices for proxies including using hardhat-deploy instead of openzepplin upgrades
  • [ ] Deploy to Testnet
  • [ ] Verify contract on Testnet
  • [x] Test Contract Upgrade Process
• [ ] Go Live Checklist
  • [ ] Testnet Deploy
  • [ ] Testnet MinServer Integration
  • [ ] Production Deploy
• [ ] Add multicall functionality for batch requests

Additional Tasks

• [ ] Add in support for ERC777. References ERC-777 Sample Code, EIP-777, OpenZepplin ERC777 Documentation, OpenZepplin ERC777.sol, OpenZepplin ERC777 Example Implementation, ERC777 Masters Thesis
  • [ ] Initial code is was done in commit https://github.com/polymorpher/sms-wallet/commit/b467f85544b89c110c00b7097b7f162df4155355 on branch jw-erc-777
• [ ] Documentation: add how to run coverage server and
  • [ ] Hardhat support for Harmony
  • [ ] Enhancements
  • [ ] Replace all requires with if tests reverts and custom errors
  • [ ] Could introduce warning events when changing thresholds if user has a balance over the new threshold
  • [ ] Optimizations
  • [ ] Review contract optimization best practices
  • [ ] Security
  • [ ] add yarn.lock to gitIgnore comments here

Outstanding Issues/Clarifications

• [ ] ERC721 safeTransferFrom not working in AssetManager.ts (using transferFrom)
• [ ] ERC1155 safeTransferFrom needs "0x" passed for data in AssetManager.ts ("" in AssetManager.sol)
• [ ] Review errorHanding for transfer (currently capture failures for ERC20 but not ERC721 and ERC1155).
• [ ] Deploy failing with unpredicatable gas limit was caused when there was a bug in the contract (not setting the admin role before setting operators) if this happens again may need to apply this workaround to fix gas price