ws-miniwallet-v0.1 MiniWallet Functionality #17

johnwhitton · 2022-09-19T21:42:39Z

Initial Commit: Summary of Changes

copy latest abi from miniwallet/build/contracts/miniWallet/MiniWAllet.sol/MiniWallet.json to miniserver/abi/MiniWallet.json
Fix logic with from and to when paying via sms (from is the funder account, to is who sent the payment request)
Add in configuration for ethlocal which is used for validation and is separate from test which is used in yarn test
Add default value for initialOperatorThreshold
Increase gasPrice for ethlocal to resolve this error Transaction's maxFeePerGas (413290302) is less than the block's baseFeePerGas (529942254) (vm hf=london -> block -> tx)

…hlocal

johnwhitton · 2022-09-19T22:54:12Z

Below Commit: Summary of changes

update config.ts, config.test.ts, env.example and .env(locally) to use HARDHAT instead of TEST for .env files
Reasoning: When testing we test both the test suite (in the miniwallet/test folder) as well as the deploy scripts (in the miniwallet/deploy folder). In order for the deploy scripts to work the configuration needs to be aligned with the network name (i.e. hardhat instead of test). Therefore we need to add this to config.ts and it is cleaner to use the config name in the .env file. As we have done this in config.ts we also updated config.test.ts to use hardhat (rather than having separate definitions for the same network. Moving forward we may be able to remove config.test.ts completely and use config.ts for testing purposes as well.
Remove operator, user and creator from config.ts
Reasoning: These are not used for miniwallet (only for minting of tokens in miniID, mini721 and mini1155). As we move forward the minting of these tokens can be separated from the deployment scripts and have a post deployment. initialization process. The functionality for initializing a chain for validaton includes
- funding the wallets of users, creators and operators
- miniwallet funding by users
- miniwallet approvals by users to creators
- Issuing miniID, mini721 and mini1155 tokens

Second Commit: Summary of Changes

Remove unused TestERC20, TESTERC721 and TESTERC1155 definitions from tests
Replace TestToken.sol with TestERC20.sol, TestERC721.sol and TestERC1155.sol in contracts/mocks. These contracts are minimial test versions generated using the open zeppelin wizard.

johnwhitton · 2022-09-19T23:56:49Z

Below Commit: Summary of Changes

Update Proxy definition to make public implementation as per feedback
Update all miniwallet testing to use proxies

polymorpher · 2022-09-20T03:39:15Z

miniserver/abi/MiniWallet.json

+{
+  "_format": "hh-sol-artifact-1",


wrong content. This should be abi only

The code was written to use the full hardhat generated json and take the abi from it see here
i.e. miniWallet = new ethers.Contract(networkConfig.miniWalletAddress, MiniWallet.abi, provider)
We can change to use abi only however when branching from main we received the error

UncaughtException [SyntaxError: /Users/john/one-wallet/sms-wallet/miniserver/abi/MiniWallet.json: Unexpected token , in JSON at position 20047]. node:internal/process/per_thread:221 throw errnoException(err, 'kill'); ^

If we prefer to use abi only need to ensure that it's JSON compliant and/or update the retrieval mechanism above.
Also should automate population of this folder when doing a yarn compile in miniwallet

Have updated the logic to use abi only
abi's are populated when compiling using hardhat-abi-exporter
The configuration is in hardhat.config.ts

abiExporter: { path: '../miniserver/abi', runOnCompile: true, clear: true, flat: true, only: [':MiniWallet$', ':MiniProxy$'], format: 'json', spacing: 2 },

Note: considered moving the abi's under a shared folder however left as miniserver only for now.

there is an extra , which I forgot to remove, just remove from the end of the JSON

polymorpher · 2022-09-20T03:41:08Z

miniserver/routes/index.js

      // Allow requesting of funds from users by address (without checking registered phone number)
      if (smsParams[1].substr(0, 2) === '0x') {
        if (!isValidAddress(smsParams[1])) {
          return respond(`error: invalid funder address ${smsParams[1]}. example request "p 0x8ba1f109551bd432803012645ac136ddd64dba72 0.1"`)
        }
-        toAddress = checkSumAddress(smsParams[1])
+        funderAddress = checkSumAddress(smsParams[1])


smsParams[1] is destination address, not "funder" (from)

In the current logic
a. the requestor is driven by the phone number
b. the funder is driven by the phone number of address.

The call to the send function is as follows

function send( uint256 amount, address from, address to

testing uses the twilio cli as follows
'twilio api:core:messages:create --from "+14158401410" --to "+17372327333" --body "p 4158401410 0.1"'
So current mapping is
sms params 0 : drives the transaction type i.e. p maps to send
sms params 1 : is the funder address (i.e. from in the send function)
sending phone number: is the to (i.e. the to in the send function)
sms params 2 : is the amount (i.e amount in the send function)

We can change this mapping if needed or document this somewhere to make it more clear.

@polymorpher can you confirm that you are happy with the above mapping.
If not can you let me know the changes or documentation that is required 🙏

Not sure whether we are on the same page of what "funder" implies? Is it not the person who initiated and funded the transaction? If so, why is smsParams[1] not the phone number of the user whom the initiator is paying to?

Sorry for the confusion. Have updated now so that the user requests the payment.
Yes the funder is the person who funded the transaction (the user) and sends the payment request.
~~But the funding request (payment) is initiated by the requestor (the creator).~~
I originally had this confused as well.
This is why we use the above mappings.

Let me try and clarify the above by adding a little more context and using the terms user and creator

user (also known as funder): someone who registers their phone and makes deposits into the miniwallet and approves those funds to be used by the creator.. For testing we use

Phone: +14158401410

Address: 0x143A933E79931006b3Eb89cBc938587546faF159

creator (also known as the requestor) : the game developer/artist who registers their phone and requests the operator to send the users approved funds . For Testing we use

Phone: +16505473175

Address: 0x58bB8c7D2c90dF970fb01a5cD29c4075C41d3FFB

Caller: TOKEN WARRIOR (used when constructing contract calls via https://demo.smswallet.xyz)

The complete scenario is as follows

The user (funder) and creator(requestor) register their phones.

The user (funder) transfers 40 ONE into their account.

The user(funder) deposits 10 ONE into the miniwallet and approves the creator to withdraw some of these funds (1 ONE).

This can be triggered by a request url such as this.

Which can be created by using the demo request page

This gives the user a request which looks as follows (once again depositing funds into the mini-wallet and approving some of those funds to be used by the creator).

The user (funder) can then check their minwallet balance by text b from their phone (+14158401410)

We test this using the twilio command

twilio api:core:messages:create --from "+14158401410" --to "+17372327333" --body "b"

And receive the following response

Your balance is 10.0 (address: 0x143a933e79931006b3eb89cbc938587546faf159)

~~5. The creator (requestor) can than ask the operator to send them some funds on behalf of the user(funder) by sending a message from their phone (+16505473175)~~

~~We test this using the twilio command~~

~~twilio api:core:messages:create --from "+16505473175" --to "+17372327333" --body "p 4158401410 0.1"~~

~~And receive the following response~~

~~Sent 0.1 (from: 0x143a933e79931006b3eb89cbc938587546faf159, to 0x58bb8c7d2c90df970fb01a5cd29c4075c41d3ffb, transaction:0x83b98c966385060236d7d7c4c7eff0d69fade1912e652c8efd24d533c20450ea)~~

The user (funder) can than ask the operator to send them some funds on behalf from the miniwallet by sending a message from their phone (+14158401410)

We test this using the twilio command

twilio api:core:messages:create --from "+14158401410" --to "+17372327333" --body "p 6505473175 0.1"

And receive the following response

Sent 0.1 (from: 0x143a933e79931006b3eb89cbc938587546faf159, to 0x58bb8c7d2c90df970fb01a5cd29c4075c41d3ffb, transaction:0x83b98c966385060236d7d7c4c7eff0d69fade1912e652c8efd24d533c20450ea)

The creator (requestor) balance is now increased by 0.1

The creator (requestor) still has no funds held in the miniwallet (the funds were sent from the miniwallet to their address)

We test the creator checking their miniwallet balance using the twilio command (i.e. from the creator (requestor))

twilio api:core:messages:create --from "+16505473175" --to "+17372327333" --body "b"

and receive the response

`Your balance is 0.0 (address: 0x58bb8c7d2c90df970fb01a5cd29c4075c41d3ffb)

The user (funders) miniwallet balance is now decreased

We test this using the twilio command

twilio api:core:messages:create --from "+14158401410" --to "+17372327333" --body "b"

And receive the following response

Your balance is 9.9 (address: 0x143a933e79931006b3eb89cbc938587546faf159)

The user (funder) sms-wallet balance after the funding and approval is as follows

After discussion with @polymorpher
The requirement is that the user calls the payment function (not the creator).
Will update the codebase accordingly.

Have updated the codebase so that user now sends payment requests.
Also updated workflow above and testing documents in FEATURE_ROLLOUT.md

polymorpher · 2022-09-20T03:43:03Z

miniwallet/config.ts

+      initialAuthLimit: ethers.utils.parseEther(process.env.ETH_LOCAL_MINIWALLET_INIITIAL_AUTH_LIMIT || '100')
+    }
+  },
+  hardhat: {


what's the difference between ethLocal and hardhat config?

ethLocal is used for end to end testing. It is part or the local testing environment documented here specifically using ganache as documented here

hardhat config is used by the following commands

yarn test or npx hardhat test

yarn deploy or npx hardhat deploy --tags deploy

Note the tags are populated in each of the test scripts
e.g. in 001_deploy_miniWallet.ts we use the following tags deployFunction.tags = ['MiniWallet', 'deploy', 'MiniWalletHardhatDeploy']
There are additional control mechanisms which can be leveraged for sequencing if needed.

polymorpher · 2022-09-20T03:43:57Z

miniwallet/contracts/mocks/TestERC1155.sol

+    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
+
+    constructor(uint256[] memory tokenIds, uint256[] memory amounts)
+        ERC1155("ipfs://testerc1155.modulo.so/")


that's invalid ipfs address

Understood.
This does not impact testing and is used as a placeholder only.
If we have a preferred ipfs adress and I can update.

@polymorpher Can you provide a better (valid) ipfs url which you’d like me to use instead of ipfs://testerc1155.modulo.so/

for example for the valid URI’s used for the mini721 and mini1155 are

MINI721_DEPLOY_BASE_URI=ipfs://Qmc8DVEthq7cZMTMyZ2NQ8dHkG99n549DMBwNzAypQgXe1/Mini721/

and

MINI1155_DEPLOY_BASE_URI=ipfs://Qmc8DVEthq7cZMTMyZ2NQ8dHkG99n549DMBwNzAypQgXe1/Mini1155/

would you like me to hardcode these values?

if the test data is not populated at an ipfs location already, empty seems more reasonable

polymorpher · 2022-09-20T03:45:53Z

miniwallet/contracts/mocks/TestERC1155.sol

+        override(ERC1155, AccessControl)
+        returns (bool)
+    {
+        return super.supportsInterface(interfaceId);


super calling wrong parent. And here it should support both ERC1155 and AccessControl interfaces instead of just one

This is a simple ERC1155 contract generated from the open zeppelin wizard
supporting

mintable

updateable uri's

roles

See this screenshot

If we need to support above requirements ERC1155 and AccessControl interfaces instead of just one we can manually write the mock contracts.

Thanks for identifying this @polymorpher 🙏
Further research confirmed your feedback and I have replaced supportsInterface with code below

function supportsInterface(bytes4 interfaceId) public view override(ERC1155, AccessControl) returns (bool) { return (ERC1155.supportsInterface(interfaceId) || AccessControl.supportsInterface(interfaceId)); }

they need to fix it

As stated above I have corrected the code for our mock Tokens.
I have also raised OpenZeppelin/contracts-wizard#183 for Open Zeppelin to fix the wizard.

polymorpher · 2022-09-20T03:46:53Z

miniwallet/contracts/mocks/TestERC721.sol

+        override(ERC721, AccessControl)
+        returns (bool)
+    {
+        return super.supportsInterface(interfaceId);


same issue here

same as above

Thanks for identifying this @polymorpher 🙏
Further research confirmed your feedback and I have replaced supportsInterface with code below

function supportsInterface(bytes4 interfaceId) public view override(ERC721, AccessControl) returns (bool) { return (ERC721.supportsInterface(interfaceId) || AccessControl.supportsInterface(interfaceId)); }

polymorpher · 2022-09-20T03:47:15Z

miniwallet/contracts/mocks/TestERC721.sol

+    }
+
+    function _baseURI() internal pure override returns (string memory) {
+        return "ipfs://testerc721.modulo.so/";


same issue here

same as above

polymorpher · 2022-09-20T03:57:13Z

miniwallet/test/utilities/index.ts

@@ -3,6 +3,10 @@ import { expect } from 'chai'
 import { MockProvider } from 'ethereum-waffle'
 import { Contract, BigNumber } from 'ethers'
 import { ethers } from 'hardhat'
+// TODO Parameterize CallData
+const ContractPath = '../../build/contracts/miniWallet/miniWallet.sol/MiniWallet.json'


should merge into one line

in fact it is not needed since you only need signature for initialize function

This can be covered as stated here

// TODO Parameterize CallData

The calldata includes both the signature for the initialize function as well as the parameters see here which is using the passed paramaters from here
i.e.

[ config.test.miniWallet.initialOperatorThreshold, config.test.miniWallet.initialOperators, config.test.miniWallet.initialUserLimit, config.test.miniWallet.initialAuthLimit ]

Have changed this to use the following line

const calldata = contract[1].interface.encodeFunctionData('initialize', contract[2] || [])

Note: all Proxy contracts we are deploying will use the initialize function so can hardcode this for now.

polymorpher · 2022-09-20T04:00:19Z

miniwallet/test/utilities/index.ts

+    const implementation = await contract[1].deploy()
+    await implementation.deployed()
+    const iface = new ethers.utils.Interface(abi)
+    const calldata = iface.encodeFunctionData('initialize', contract[2] || [])


could use generic encoding function similar to one-wallet lib util encodeCalldata

Great point 🙏

encodeCalldata

// WARNING: returns string-encoded bytes (0x....), unlike other functions provided in this package encodeCalldata: (method, values = []) => { if (!method) { return '0x' } const selector = abi.encodeFunctionSignature(method) const m = method.match(/.+$(.*)$/) if (!m) { return null } const params = m[1] ? m[1].split(',') : [] const encodedParameters = abi.encodeParameters(params, values) return selector + encodedParameters.slice(2) },

We should consider how it is best to leverage the existing one-wallet lib.
Perhaps writing this as an npm package may be one solution.

I reviewed the oneWallet encodCallData above.

If I understand it correctly if would need
method = 'initialize(uint8, address[], uint256, uint256)

params = [ config.test.miniWallet.initialOperatorThreshold, config.test.miniWallet.initialOperators, config.test.miniWallet.initialUserLimit, config.test.miniWallet.initialAuthLimit ]

And used this one-liner which is generic for all Proxy deploys and Implementation upgrades
const calldata = contract[1].interface.encodeFunctionData('initialize', contract[2] || [])

which translates to

const calldata = contract[1].interface.encodeFunctionData( 'initialize', [ config.test.miniWallet.initialOperatorThreshold, config.test.miniWallet.initialOperators, config.test.miniWallet.initialUserLimit, config.test.miniWallet.initialAuthLimit ]

The encodeFunctionData seems slightly simpler as it just needs the method and will figure out the data types

Moving forward could create a generic function taking input as (abi, method, params) or alternately contract instance, method params) if needed.

okay, importing the whole abi seems excessive if it is just for getting the signature of initialize function. It is known anyway

polymorpher · 2022-09-20T04:02:15Z

miniserver/.env.example

@@ -46,7 +46,7 @@ POLLING_INTERVAL=1000
 DEFAULT_NETWORK='eth-local'
 GAS_LIMIT='67219000'
 #GAS_PRICE='10000000'
-GAS_PRICE='413290302'
+GAS_PRICE='529942254'


should use type 2 transactions instead of arbitrary numbers, or manually set base fee in emulating local eth blockchain

Good Point 🙏
An example of how to use type 2 transactions would be useful
Initial google shows these EIPS

EIP-1559: A transaction pricing mechanism that includes fixed-per-block network fee that is burned and dynamically expands/contracts block sizes to deal with transient congestion.

EIP-2178: TransactionType || TransactionPayload is a valid transaction and TransactionType || ReceiptPayload is a valid transaction receipt where TransactionType identifies the format of the transaction and *Payload is the transaction/receipt contents, which are defined in future EIPs.

Will investigate further.

Further Investigation
Gives this code example

require("log-timestamp"); const ethers = require("ethers"); const privateKey = ("ADD_YOUR_PRIVATE_KEY_HERE").toString('hex'); const wallet = new ethers.Wallet(privateKey); const address = wallet.address; console.log("Public Address:", address); const httpsUrl = "ADD_YOUR_HTTP_URL_HERE"; console.log("HTTPS Target", httpsUrl); const init = async function () { const httpsProvider = new ethers.providers.JsonRpcProvider(httpsurl); let nonce = await httpsProvider.getTransactionCount(address); console.log("Nonce:", nonce); let feeData = await httpsProvider.getFeeData(); console.log("Fee Data:", feeData); const tx = { type: 2, nonce: nonce, to: "0x8D97689C9818892B700e27F316cc3E41e17fBeb9", // Address to send to maxPriorityFeePerGas: feeData["maxPriorityFeePerGas"], // Recommended maxPriorityFeePerGas maxFeePerGas: feeData["maxFeePerGas"], // Recommended maxFeePerGas value: ethers.utils.parseEther("0.01"), // .01 ETH gasLimit: "21000", // basic transaction costs exactly 21000 chainId: 42, // Ethereum network id }; console.log("Transaction Data:", tx); const signedTx = await wallet.signTransaction(tx); console.log("Signed Transaction:", signedTx); const txHash = ethers.utils.keccak256(signedTx); console.log("Precomputed txHash:", txHash); console.log(`https://kovan.etherscan.io/tx/${txHash}`); httpsProvider.sendTransaction(signedTx).then(console.log); }; init();

Also debugging eth_estimateGas implies that we are using type2 transactions "type":"0x2",

requestBody: '{"method":"eth_estimateGas","params":[{"type":"0x2","maxFeePerGas":"0x77359400","maxPriorityFeePerGas":"0x77359400","from":"0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266","to":"0xe7f1725e7734ce288f8367e1bb143e90bb3f0512","data":"0x3d0594d4000000000000000000000000000000000000000000000000016345785d8a0000000000000000000000000000143a933e79931006b3eb89cbc938587546faf15900000000000000000000000058bb8c7d2c90df970fb01a5cd29c4075c41d3ffb"}],"id":49,"jsonrpc":"2.0"}',

For now I've updated prepareExecute to retrieve the gasPrice
const gasPrice = await provider.getGasPrice()
This introduces an additional rpc call on each transaction
Note this uses getGasPrice there is also estimateGas which may give a more accurate estimate.

@polymorpher let me know your thoughts on this and whether the additional rpc call is to burdensome, and if you'd prefer to use estimateGas

Also assumption is that this works on all evm compatible chains (Including Harmony) however this has only been tested on hardhat and ganache.

that's fine, type 2 is not supported on all chains, including ganache

johnwhitton · 2022-09-23T17:10:14Z

This PR is now at a logical point to merge.
My recommendation is to merge this PR and progress additional items under ws-miniwallet-v0.2
The additional items (which I'll begin working on now) are

Update to use native deployment scripts and remove ADMIN_SLOT logic in MiniWallet.sol
Persist deploy artifacts replacing hardhat-deploy generated artifacts
Finalize Deterministic Deployment Approach for Contracts
Finalize supportsInterface logic for Mock contracts latest feedback from OpenZeppelin
Optional - replace hardhat-abi-exporter with our own code

polymorpher · 2022-09-24T07:27:51Z

miniserver/routes/index.js

@@ -39,47 +39,50 @@ const parseSMS = async (req, res, next) => {
    res.status(status).send(response.toString())
  }
  try {
+    let fromAddress


no reason this should be mutable

Fixes to ensure miniwallet and miniserver can be validated against et…

4132c59

…hlocal

johnwhitton added 2 commits September 19, 2022 15:54

Update config so that testing and deploy work on hardhat

83e51d1

Update Test Tokens in mock and remove unused tokens in tests

e493ae3

johnwhitton added 2 commits September 19, 2022 18:41

Improve tests to use Proxies

6a00b8e

Update FEATURE_ROLLOUT.md to include items for MiniID's and MiniNFTs

07b9ee2

polymorpher reviewed Sep 20, 2022

View reviewed changes

johnwhitton added 5 commits September 21, 2022 11:54

Update supports interface in TestERC721.sol and TestERC1155.sol

f82d812

Retreive gasPrice when executing miniserver transactions

7ef7989

improve getCallData and population of abis

357dec7

Remove dummy URI's in Mock Contracts

b0267e6

Update Miniserver payment api, so that request comes from user

9b66e2e

polymorpher reviewed Sep 24, 2022

View reviewed changes

polymorpher added 2 commits September 24, 2022 00:37

fix variable mutability

f2060a7

documentation revision

dc54f9d

polymorpher approved these changes Sep 24, 2022

View reviewed changes

polymorpher merged commit 35b30d5 into main Sep 24, 2022

ws-miniwallet-v0.1 MiniWallet Functionality #17

ws-miniwallet-v0.1 MiniWallet Functionality #17

Conversation

johnwhitton commented Sep 19, 2022 • edited Loading

johnwhitton commented Sep 19, 2022 • edited Loading

johnwhitton commented Sep 19, 2022 • 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

Choose a reason for hiding this comment

johnwhitton Sep 22, 2022 • 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

johnwhitton Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwhitton Sep 20, 2022 • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwhitton Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

johnwhitton Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwhitton commented Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

johnwhitton commented Sep 19, 2022 •

edited

Loading

johnwhitton commented Sep 19, 2022 •

edited

Loading

johnwhitton commented Sep 19, 2022 •

edited

Loading

johnwhitton Sep 22, 2022 •

edited

Loading

johnwhitton Sep 20, 2022 •

edited

Loading

johnwhitton Sep 20, 2022 •

edited

Loading

johnwhitton Sep 21, 2022 •

edited

Loading

johnwhitton Sep 21, 2022 •

edited

Loading

johnwhitton commented Sep 23, 2022 •

edited

Loading