Mini v0 by johnwhitton · Pull Request #14 · polymorpher/sms-wallet

johnwhitton · 2022-09-04T21:51:30Z

Version 0 work based on #13

…eedback from PR 7

…using mnemonic

…ving to shared)

polymorpher · 2022-09-12T08:25:24Z

miniwallet/deploy/001_deploy_miniWallet.ts

+    from: deployer,
+    proxy: {
+      owner: deployer,
+      proxyContract: 'EIP173Proxy',


where is EIP173Proxy?

This was a simple proxy used by hardhat-deploy. We have update this now to use ERC1967Proxy.sol which is UUPS compatible and is held under the deployments folder

polymorpher · 2022-09-12T08:26:59Z

miniwallet/deploy/001_deploy_miniWallet.ts

+  }
+
+  console.log('operators:', JSON.stringify(initialOperators))
+  const deployedContract = await deploy('MiniWallet', {


Where can we control and store deployed proxy address, storage address, and logic contract address?

polymorpher · 2022-09-12T08:28:34Z

miniwallet/deploy/002_deploy_miniID.ts

+  console.log('MiniID Name         : ', await miniID.name())
+  console.log('MiniID Symbol       : ', await miniID.symbol())
+  // Mint test tokens
+  if (chainId !== '1666600000,') {


Refactored approach and commit are here

polymorpher · 2022-09-12T08:29:30Z

miniwallet/deploy/003_deploy_mini721.ts

+  let contractUri
+
+  console.log(`chainId: ${chainId}`)
+  if (chainId === '1666600000,') {


and no hard coding

Refactored approach and commit are here

polymorpher · 2022-09-12T08:29:45Z

miniwallet/deploy/003_deploy_mini721.ts

+  console.log('Mini721 Symbol       : ', await mini721.symbol())
+
+  // Mint test tokens
+  if (chainId !== '1666600000,') {


Refactored approach and commit are here

polymorpher · 2022-09-12T08:31:00Z

miniwallet/deploy/003_deploy_mini721.ts

+
+  const deployedMini721 = await deploy('Mini721', {
+    from: deployer,
+    gasLimit: 4000000,


Removed 🙏
Was initially put in as was getting unexpected gas limit, but this was due to errors in deploy logic

polymorpher · 2022-09-12T08:31:46Z

miniwallet/deploy/004_deploy_mini1155.ts

+  let contractUri
+
+  console.log(`chainId: ${chainId}`)
+  if (chainId === '1666600000,') {


same issues

Refactored approach and commit are here

polymorpher · 2022-09-12T08:31:58Z

miniwallet/deploy/004_deploy_mini1155.ts

+  console.log('Mini1155 Symbol       : ', await mini1155.symbol())
+
+  // Mint test tokens
+  if (chainId !== '1666600000,') {


same issues

Refactored approach and commit are here

polymorpher · 2022-09-12T08:33:36Z

miniwallet/deploy/101_upgrade_miniWallet.ts

+    from: deployer,
+    proxy: {
+      owner: deployer,
+      proxyContract: 'EIP173Proxy'


same issue here, not sure where eip173proxy is and whether it is safe and how it can be controlled

This was a simple proxy used by hardhat-deploy. We have update this now to use ERC1967Proxy.sol which is UUPS compatible and is held under the deployments folder

polymorpher · 2022-09-12T08:35:22Z

Can you gitignore everything under /deployment? No auto-generated code or local-environment specific stuff should be checked in

polymorpher · 2022-09-12T08:37:01Z

Can all Mini1155 and 721 related deployment and testing stuff be removed? They are launch templates other developers may use, and have nothing to do with SMS Wallet features. The contract themselves may be moved to separate places as well

polymorpher · 2022-09-12T08:38:22Z

Can you move all work-in-progress and MiniID related items to separate PR? Let's make one thing perfectly good, solid for production, and as minimal as possible in code, before moving on to the next.

polymorpher · 2022-09-12T08:42:23Z

There is still the issue unresolved, related to recording proxy, logic, and storage addresses for those hardhat deployed contracts, and fine-control on these contract's upgrade process. We can't just use deploy or upgrade without knowing or controlling each and every step what it is doing. Are they using factories and create2? Using EOA and create op code? In some situations we also need to control the address which the contracts are deployed to. We need to know all these. In other situations when we upgrade the contracts, the storage slots may mess up because of storage layout issues, and we need to test ahead of time. We cannot do this without complete visibility and fine-control over the deploy and upgrade process

polymorpher · 2022-09-12T08:44:21Z

miniwallet/test/Mini1155.ts

@@ -0,0 +1,489 @@
+import { expect } from 'chai'


Is there any way we can continue the review of these files in the other PR where they were originally reviewed? Several hours were spent reviewing the files, so it would be nice to continue from existing reviewing checkpoint, rather than starting from scratch

I went through the code again this time. Let's keep reviewed PRs open in the future

server/routes/middleware.js

polymorpher · 2022-09-12T08:47:47Z

miniwallet/test/utilities/Mini721Util.ts

+  })
+}
+
+export async function communityMint721 (testEnvironment, owner, numTokens) {


is this needed?

polymorpher · 2022-09-12T08:48:36Z

miniwallet/test/utilities/Mini721Util.ts

@@ -0,0 +1,39 @@
+import { ethers } from 'hardhat'
+
+export const ADDRESS_ZERO = '0x0000000000000000000000000000000000000000'


this is in server config, it can be moved to a constant in a single place

polymorpher · 2022-09-12T08:57:22Z

Let's also move development notes to miniwallet/devlog/ with a README stating they are development notes for internal discussions, not meant for documentation or guiding users or developers.

We can polish and move things out to component-root folder after they are ready for production and presentable for external audience

polymorpher · 2022-09-12T09:01:48Z

miniwallet/test/Mini1155.ts

+      expect(await this.mini1155.balanceOf(owner, c1.s.tokenId)).to.equal(0)
+      expect((await this.mini1155.balanceOfBatch([owner, owner], [1, 2])).map(x => x.toString())).to.deep.equal(['0', '0'])
+      expect(await this.mini1155.baseUri()).to.equal('ipfs://QmPcY4yVQu4J2z3ztHWziWkoUEugpzdfftbGH8xD49DvRx/')
+      expect(await this.mini1155.contractURI()).to.equal('ipfs://QmdKB6d1zT7R8dNmEQzc6N1m5p2LDJZ66Hzu8F4fGhdVrq')


these specific project urls all need to be sanitized and removed from tests and default config parameters

polymorpher · 2022-09-12T09:12:55Z

I have two more files to read: NFTID.md and PROXY.md. I went through other files.

Let's break down this PR into multiple parts (each has its own PR):

core mini-wallet (server, contract, tests) - these are ready for merge
mini-wallet deployment and upgrade scripts - these needs some improvement on fine-control over deploy and upgrade process (most likely can't just use simple hardhat calls / plugins anymore) and storing (not just logging) the addresses of proxy, logic, and storage
mini1155, 721, and related deployment scripts and tests - these need some sanitization and maybe some simplification on tests. They are pretty much ready for merge
miniID related and other WIP stuff - these are incomplete
documentations (NFTID.md, PROXY.md) - these need further review and some revision - I can also edit on the branch itself

The PRs can be made in chains (each PR's branch merge into the branch of previous PR) rather than all pointing to the base (main) branch, since each may be dependent on some of the work in previous PR

polymorpher · 2022-09-12T17:10:45Z

On a second thought - I think (2) can be merged in after using a better proxy, so we can get it up and running ASAP. We can improve this process in a later PR

…et and MiniId

…NFTs and improve deployment and testing configuration

polymorpher · 2022-09-14T22:39:13Z

miniwallet/config/getConfig.ts

+  let config
+  if (fs.existsSync(configPath)) {
+    configPath = configPath.substring(0, configPath.length - 3)
+    const { default: configNetwork } = await import(configPath)


could be used to executed malicious script in temporary / mounted / remote folders

polymorpher · 2022-09-14T22:40:43Z

miniserver/routes/middleware.js

+      return res.status(StatusCodes.BAD_REQUEST).json({ error: `phone number not registered: ${phoneNumber}` })
+    }
+  }
+  console.log(`u: ${JSON.stringify(u)}`)


need to remove - leaking user info in console

removed 🙏

johnwhitton added 13 commits August 21, 2022 20:22

Initial version of miniwallet server

c0473e2

Add in sms message parsing and responses in twilio format

ac251c4

Fix logic error in funding where funder and recipient were backwards

e45388c

WIP: Begin work on Miniserver multiple operators per node and other f…

a234cf9

…eedback from PR 7

WIP add executor logic to blockchain.js to support multiple accounts …

70ee740

…using mnemonic

Working version of executor supporting multiple accounts via mnemonic

4b988b4

Remove miniserver src/data files and use servers for now (consider mo…

a47c25b

…ving to shared)

Improve error handling for miniserver

86c7933

rename AssetManager to MiniWallet

9d4ab08

Make MiniWallet Approval function payable and call deposit

b55e00c

Add in Logic for Proxy Upgrades for miniwallet

5450eeb

Initial Scaffolding for NFTs Mini721 and Mini1155 contracts

559f191

Clean up server and deploy logic (Feedback from PR 7 and 8)

5990a9d

johnwhitton mentioned this pull request Sep 4, 2022

Initial version of miniwallet server #7

Closed

Remove unused config in miniserver

21ae963

johnwhitton mentioned this pull request Sep 4, 2022

Miniwallet enhancements (including miniserver and smart contract changes) #8

Closed

Clean up MiniWallt_v2.sol and redeploy locally

de7e7c5

johnwhitton mentioned this pull request Sep 4, 2022

MiniWallet Contracts refactor, MiniServer, Proxy Upgrade Deploys #12

Closed

4 tasks

Rename HRC721 to ERC721M (for MiniWallet)

875227f

johnwhitton mentioned this pull request Sep 5, 2022

MiniWallet - Launch Task List #13

Open

4 tasks

johnwhitton added 3 commits September 4, 2022 19:54

NFT Identity improvements around testing and config

a63d850

Merge branch 'main' into miniV0

56d8d31

MiniWallet NFT Testing remove unused code and improve readability

b4b87d1

johnwhitton mentioned this pull request Sep 5, 2022

Contains MiniWallet Contracts refactor, MiniServer, Proxy Upgrade Deploys and NFT Scaffolding #11

Closed

johnwhitton added 6 commits September 5, 2022 14:12

Add in Deploy for Mini721 and update configuration

534e54f

Draft NFTId design

c746fef

NFTID.md add additional reference info

5ae56da

Add MiniID contract and update Proxy Deployments

c969982

Initial scaffolding for MiniID API

80c707c

Initial scaffolding for MiniID API (all files)

79510f8

polymorpher reviewed Sep 12, 2022

View reviewed changes

server/routes/middleware.js Outdated Show resolved Hide resolved

polymorpher reviewed Sep 12, 2022

View reviewed changes

server/routes/middleware.js Outdated Show resolved Hide resolved

polymorpher reviewed Sep 12, 2022

View reviewed changes

johnwhitton added 5 commits September 12, 2022 17:59

Implement our own ERC1967 Proxy and test upgrade process for MiniWall…

1ad5a46

…et and MiniId

Move NFTID and Proxy design docs into devlog

1aeb0c7

Refactor miniWallet for different streams miniWallet, miniId and Mini…

f851991

…NFTs and improve deployment and testing configuration

Fix minor mistake in Miniwallet CONFIGURATION.md

696d0e2

Clean up miniwallet configuration based on miniV0 feedback

38a01cb

polymorpher reviewed Sep 14, 2022

View reviewed changes

johnwhitton mentioned this pull request Sep 17, 2022

ws-miniwallet-v0 MiniWallet Functionality #16

Merged

		@@ -0,0 +1,39 @@
		import { ethers } from 'hardhat'

		export const ADDRESS_ZERO = '0x0000000000000000000000000000000000000000'

Comments

Conversation

johnwhitton commented Sep 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polymorpher Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polymorpher commented Sep 12, 2022

Uh oh!

polymorpher commented Sep 12, 2022

Uh oh!

polymorpher commented Sep 12, 2022

Uh oh!

polymorpher commented Sep 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polymorpher Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polymorpher commented Sep 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polymorpher commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polymorpher commented Sep 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

polymorpher Sep 12, 2022 •

edited

Loading

polymorpher Sep 12, 2022 •

edited

Loading

polymorpher commented Sep 12, 2022 •

edited

Loading