search

polymorpher / sms-wallet

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

Discussions related to wiki #2

Closed polymorpher closed 2 years ago

polymorpher commented 2 years ago

Below comments are submitted by John in the initial full-spec Google Doc (which got moved to Wiki). I am consolidating them here for further discussions

Re: mini-wallet, withdraw

withdraw(uint256 amount): allows the user to withdraw native assets the user previously deposited into the contract.

  1. If the user passes a zero amount all user funds should be withdrawn
  2. Withdrawal also reduces the amount authorized down by the amount withdrawn.

send(uint256 amount, address from, address to): allows the operator to send a certain amount of native tokens deposited by user from to address to, provided that user from has already authorized more than such amount of native tokens and the limit is not used up.

  1. Send also reduces the amount authorized by the sent amount.
  2. Send can only be called by the owner of the contract.

transfer(uint256 amount, uint256 tokenId, address contract, address from, address to): similar to send, but works for ERC20/721/1155 tokens. Note that approval functions (equivalent to pre-authorization) of the tokens reside on the token-contracts themselves, not this smart contract.

  1. Assumption is that the user will either: a. deposit the ERC20/721/1155 tokens into the AssetManager and then only the owner of the AssetManager can send these tokens on the users behalf. or b) Approve the AssetManager for the tokens the user wishes to transfer (e.g. https://docs.openzeppelin.com/contracts/2.x/api/token/erc721#IERC721-approve-address-uint256-) and then only the owner of the AssetManager can send these tokens on the users behalf.
polymorpher commented 2 years ago

re: withdraw - I agree with both

re: send - I agree with (1) but not entirely for (2). The function should be called by the operator instead of the owner. The permissions related to the owner and the operator should be separate. We may want to reserve the use of owner account for high-risk operations only (e.g. upgrading the contract, pause all activities, global per-user mini-wallet limit, and others)

re: transfer - we should use approval only instead of having the users to transfer the ownership over the token to the contract. The latter approach (transfer ownership) would make the token disappear from the user's list of owned tokens, which is not what the user wants.

johnwhitton commented 2 years ago

Based on the feedback here and in the code review for https://github.com/polymorpher/sms-wallet/pull/1 I've added the following additional tasks (requirements)

johnwhitton commented 2 years ago

Design Notes Add global per-user mini-wallet limit(owner function) GLOBAL AUTH LIMIT 100 (max they can authorize per destination)

uint256 public globalUserAuthLiimit; //Maximum a user can authorize

Add user limits for destination addresses (not just a global limit) Implementation Options

  1. Use two-level mapping as suggested by @polymorpher below example from ERC20 is here 

    mapping(address => mapping(address => uint256)) private _allowances;

  2. Key for mapping containing both sender and Recipient mapped to an amount 

    mapping(string => uint256) public userAuthorizations;
sample data
ALICE-GAMEA -> 10
ALICE-GAMEB -> 20
ALICE-GAMEC -> 30

  3. Key contains sender and maps to an array of structure containing [recipient and amount]

  struct auths {
    address recipient;
    uint256 amount;
  }
mapping(address => auths[]) public userAuthorizations;
sample data
ALICE -> [[GAMEA,10],[GAMEB,20],[GAMEC,30]]

Recommendation is to go with option 0. Previous recommendation of option 2 is overly complicated and inefficient, thanks @polymorpher Although this complicates the storage and retrieval of the authorization. It has the benefit of being able to retrieve all authorizations for a sender. Which is useful if when we need to check total_authorizations for a sender.

Gist for option 2 (outdated) uses a mapping (so that we can look up the users quickly and an array of arrays to store the recipient limits)

    struct RecipientAuth {
        address recipient;
        uint256 limit;
        }
    struct UserRecipientAuth {
        address user;
        RecipientAuth[] recipientAuths;
        }

    UserRecipientAuth[] userRecipientAuths;

    mapping(address => uint256) public userBalances;
    mapping(address => uint) public userAuthorizations;

Error with first Attempt at option 2 This was using a mapping storing an array of recipient values FirstDraft of AssetManager.sol Compile error gist

    struct userAuth {
        address recipient;
        uint256 limit;
        }
    userAuth[] userAuths;
    mapping(address => uint256) public userBalances;
    mapping(address => userAuth[]) public userAuthorizations;

References Reference Mapping Types Solidity Documentation Reference Question multi attribute query StakeOverflow Reference Question multi value mappings StackOverflow Reference Question memory vs storage StackOverflow

polymorpher commented 2 years ago

Have you considered using two-level mapping? It is common in ERC20,721,1155 contract templates. It is a lot more efficient than using a single level mapping to an array

The first error you had is because you were creating a temporary storage variable, which does not make sense because storage variables are meant to be persisted. You need to change that to a memory variable. To resolve the second error you need to copy the elements one by one and extend the storage array if needed.

johnwhitton commented 2 years ago

Design Notes

TODO

polymorpher commented 2 years ago

I think we can close this off for now. ERC777 is not a priority right now

polymorpher commented 2 years ago

I will add a note to update the wiki later per this discussion and actual implementations