Project - Multisig Wallet

i had to watch the tips but i didn’t use the double mapping. nor the approval to boolean. I’m sure my wallet is not very good, it seems to work though. although i just realised one person can approve twice and it will count it right now as 2 i think.
but i kinda was already proud of myself of what i made here (i do not have previous programming education) … now i feel sad haha. i started with a fresh new file so i made everything new.

pragma solidity 0.7.5;
pragma abicoder v2;

contract Bank {

address owner1;
address owner2;
address owner3;

    // not used  mapping (address => mapping(uint => bool)) approvals;

uint balance;
uint amountsign;
struct TX {
    uint txnr;
    //address txfrom;
    address txto;
    uint approvalsreceived;
    uint amounttx;
}

TX[] transaction;
  constructor (address _owner2, address _owner3, uint _amountsign)  {
owner1 = msg.sender;
owner2 = _owner2;
owner3 = _owner3;
 amountsign = _amountsign;
}

function deposit () public payable {
    balance += msg.value;
    //of balance[this] ?
}

function getBalance () public view returns (uint){
    return address(this).balance;
    //return address(this).balance;
}

function addTransaction (address payable _to, uint _amount) public  {
    //of vanuit this.balance;
   // require(isCoOwner == true);
    require (address(this).balance >= _amount, "you cannot transfer that much");
    require (owner1 == msg.sender || owner2 == msg.sender || owner3 == msg.sender ) ;
    TX memory newTX = TX(transaction.length, _to, 0, _amount);
    transaction.push(newTX);
}

  function viewtransaction2 (uint _txnr)  public view returns (TX memory){
    return transaction[_txnr] ;
}
  
function approveTX (uint _txnr)  payable public {

   require (owner2 == msg.sender || owner3 == msg.sender ) ;
    transaction[_txnr].approvalsreceived += 1;
    address payable TransferTo = payable (transaction[_txnr].txto);
    if (transaction[_txnr].approvalsreceived >= amountsign) {
 
   balance -= transaction[_txnr].amounttx;
    TransferTo.transfer(transaction[_txnr].amounttx);
    }
}

function getapprovalamount (uint txnr) public view returns (uint) {
    return transaction[txnr].approvalsreceived ;
}

}

2 Likes

Hi @B_S,

Just to clarify the other use of the following syntax …

Sometimes we might have a situation where we need to create a new local array of values (a memory array) which doesn’t need to be saved to storage. Instead, it only needs to be returned once the function has finished adding values to it. This can occur when, within a function, we perform a series of iterations, often over another array already saved in persistent storage (a storage array), in order to generate a new group of values, or to select a subset of pre-existing ones. The important point to understand, here, is that, depending on the type of computation, we may or may not know the number of values the new array will end up containing once the interations have finished and the array is ready to be returned.

The issue we are faced with is that, unlike a storage array, a local memory array must have its length defined before any values can be added to it. If each time a specific function is called it will always generate and return a local memory array containing the same number of values, then we can simply define a fixed-size memory array e.g.

address[5] memory newArray;

However, if the number of values contained within each new array will vary depending on some other pre-definable variable or parameter (lets, call it num ), then we can’t define our local memory array as follows…

address[num] memory newArray;  // we can't do this

Instead, we can define a dynamically-sized memory array, and assign to it a new instance of a fixed-length array whose fixed-length can change each time the function is executed according to the value of a specified variable or parameter. This means that the length is ony fixed temporarily until the function has finished executing. ( See below: contract NumberSets_ A, function selectSubSet )

address[] memory newArray = new address[](num);

We can also define a local memory array in this way, add a different number of values to it each time the function is called, and then save it to storage as part of a newly created struct instance, as one of its properties.  ( See below: contract NumberSets_ B, function createInitial )
This is an alternative to first creating the new struct instance and saving it to storage with an empty array as one of its properties, and then using .push() to add the values to the array before the function has finished executing.  ( See below: contract NumberSets_ A, function createInitial )

Below are two contracts I’ve created to demonstrate how both of these alternative uses of the syntax we’ve been discussing can actually be implemented. Both contracts create an initial range of numbers, which is generated by an algorithm based on a number argument (num) between 10 and 20.
num determines (i) how many numbers are in the range, (ii) the interval between each number, and (iii) the first number in the range: num x 3

From this initial range, a subset can then be created, which is generated by an algorithm based on another number argument (freq) between 2 and 5.
freq determines (i) which position in the initial range is selected as the first number in the setset, (ii) the frequency at which the rest of the numbers are selected from the initial range, starting from the position of the first selected number, and (iii) how many numbers are in the subset:
how many numbers in initial range ÷ freq (rounded down to nearest whole number)

The algorithms themselves aren’t meant to represent any kind of real-world scenario. They are purely designed to help create a simple coding context which requires (i) the creation of struct instances which contain a dynamic array as one of their properties, and which are saved in persistent storage in a mapping, and (ii) for-loop iteration to assign varying quantities of values to both memory and storage arrays.

Both contracts essentially generate the same results. The difference is in the code they use to achieve that.

  • Contract NumberSets_A uses …

    • new address[](0)    to create the initial ranges of numbers; and
    • new address[](num)   to select the numbers and generate the subset arrays
  • Contract NumberSets_B uses each alternative form of our syntax to perform the opposite task to the one they perform in contract NumberSets_A …

    • new address[](num)   to create the initial ranges of numbers; and
    • new address[](0)    to select the numbers and generate the subset arrays
// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

contract NumberSets_A {
    
   struct Record {
      uint number;
      uint[] myArray;
   }
    
   mapping(address => Record) initial;
    
   function createInitial(uint num) public {
      require(num >= 10 && num <= 20, "Number must be between 10 & 20");
      initial[msg.sender] = Record(num, new uint[](0));
        
      uint generator = num * 3;
      for(uint8 i = 0; i < num; i++) {  
         initial[msg.sender].myArray.push(generator);
         generator += num;
      }
   }
    
   function getInitial() view public returns(uint, uint[] memory) {
      return (initial[msg.sender].number, initial[msg.sender].myArray);
   }
    
   function selectSubSet(uint freq) view public returns(uint[] memory) {
      require(freq >= 2 && freq <= 5, "Frequency must be between 2 & 5");
      uint num = initial[msg.sender].myArray.length / freq;
      uint[] memory array = new uint[](num);
        
      uint index = freq - 1;
      for(uint8 i = 0; i < num; i++) {
         array[i] = initial[msg.sender].myArray[index];
         index += freq;
      }
        
      return array;
   }
    
}
// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

contract NumberSets_B {
    
   struct Record {
      uint number;
      uint[] myArray;
   }
    
   mapping(address => Record) initial;
   mapping(address => Record) subSet;
    
   function createInitial(uint num) public {
      require(num >= 10 && num <= 20, "Number must be between 10 & 20");
      uint[] memory array = new uint[](num);
        
      uint generator = num * 3;
      for(uint i = 0; i < num; i++) {  
         array[i] = generator;
         generator += num;
      }
        
      initial[msg.sender] = Record(num, array);
   }
    
   function getInitial() view public returns(uint, uint[] memory) {
      return (initial[msg.sender].number, initial[msg.sender].myArray);
   }
    
   function selectSubSet(uint freq) public {
      require(freq >= 2 && freq <= 5, "Frequency must between 2 & 5");
      uint num = initial[msg.sender].myArray.length / freq;
      subSet[msg.sender] = Record(num, new uint[](0));
        
      uint index = freq - 1;
      for(uint i = 0; i < num; i++) {
         uint selectedNum = initial[msg.sender].myArray[index];
         subSet[msg.sender].myArray.push(selectedNum);
         index += freq;
      }
   }
    
   function getSubSet() view public returns(uint, uint[] memory) {
      return (subSet[msg.sender].number, subSet[msg.sender].myArray);
   }
    
}

@mcgrane5, @thecil

3 Likes

question to the teacher: i was a bit confused about if I could do address this balance for the sending. what i am confused about is: there will be a contract made per initial msg.sender who says make this a contract right? or not? so, basically, can you use this balance or should you have a balance mapping? i didn’t understand that because i started out with that but then saw that if you are not the creator of the contract and you want to do a transaction you cannot do it because you have no funds if you have used mapping…

1 Like

@minke you should really use a balance mapping. Because if someone creates a contract say personA. and personA deposits 10 ether into the contract. The personA has a balance of 10 ether and the total contract has a balance of 10 ethr. However if personB comes along and deposits 5 ether. Then the contract balance goes up to 15 ether but the entire contract balance is distributed amoung personA and personB. so personA owns 10, personB owns 5 and thus 10 + 5 = 15 = contract balance. Thus using address(this).balance to transfer would not be correct as your playing with the contractb balance here not the individual user balances.

You can think of the balance mapping as an artifical construct or tool that lets us figure out which owner owns what portion of the contract balance. The actual value stored in the mapping is not ether or does not have any monetary value. It is only a representation of how much an indivdual owns out of the total contract balance, thus we use to for this purpose.

1 Like

very very impressive work @jon_m, you really went the extra mile here and i learned a thing or two myself from your points on local and memory arrays. It actually reminds of just like in C++ where do create a dynamic arrray you need to allocate memnory succificently depending on the value you choose to set the array length at (N) by using malloc to multily the number of elemtns by the memory size of the varibale your using such as N * 8btyes for the case of an array of doubles. In C++ things like this are made more apparent because your actually manually defining how much memory you gonna need so your always aware of whatb happening but in some higher level languages like solidity the same rules still apply but you neevr have to be as concerned with the lower level operations because you dont have to actually do the excat memory calcs which can make “things” just work (or not work) without knowing whats going on under the hood. Very good post this refreshed my “memory” lol on all of this type of stuff. thansk for that

2 Likes

Hi Mcgrane5,

thanks so much! that was indeed was i was suddenly confused about: whether the contract-address would be made per person who makes it or if it is one contact that everyone would use. Now after your explanation i suddenly remember. Then I have to now rethink how you can make it that if person A sets up the contract for itself + person b and person c with min. 1 approval and person b wants to make a transaction … how would you send from the shared wallet… since it is not the original msg sender… or maybe i misunderstood and only the original msg.sender was supposed to be able to make a transaction.

o wait perhaps in the transaction you just have to input the address of the original msg.sender as “_from” ? is that it…? but that would be weird… but if i was not personA it won’t see a balance while i can transfer… oi this is quite a puzzle :slight_smile:

1 Like

ok so your on about how to set up the approval system yes? ok so what you should do is make a transfer struct. like so. not we will also need a double mapping that allows us to check if a given owner has prroved a given transafer

mapping(address => mapping(uint => bool) approvals;
//note we index this mapping like
//approvals[msg.sender][transferId] = true/false
//we need the double mapping to allow us to check two things those being "has this address" 
//"approved this transfer" return yes or no

uint transferId = 0;

struct Transfer {
    address from;
    address payable to;
    uint amount;
    uint transferId
    uint approvals
    uint timeOfTransaction
}

Trnasfer[] transferRequests;

so let me just explain what structs are. Structs are user defind dataTypes. So just thnk of a uint (unisnged interger) or and address for a moment. Those two are dataTypes. Therefro in solidity if we want to declare a number varaible we need to “typecast” it to “uint”. If we want to make an address variable we need to typecast it to address as you know. Here what a struct lets you do is to defin eyour own custome data type. Then thats why i say Transfer[] transferRequests because we are declaring an array of “type” Transfer. This array will store all of our transfers in this array.

To make a transfer you should define a function called createTransfer() or something like that. In this function we will make an “instance” of the transfer struct above and push it to the transferRequest array. we can achieve that like so

function createTransfer(address payable receiver, uint amount) public onlyOwners {
    
    transferRequests.push(Transfer(msg.sender, receiver, amount, 0, transferId, block.timestamp));
    transferId += 1;
}

Note that the above function is a skeleton you should add more bits such as require statemetns etc but i just want to highlught the instanciation of the transfer request and pushing it to the array. Note how we originally set the number of approvals to zero here because we know that when we create a transfer we wont have any approvals.

Ok so now we have made our transfer request. But how do we go about approving it. well we can make an approval function for that purpose.

function approveTransfer(uint id) public onlyOwners {

   require(approvals[msg.sender][id] == false, "cannot approve same transfer twice");
   
   approvals[msg.sender][id] =  true;
   transferRequests[id].approvals += 1;

   if (transferRequests[id].approvals == limit) {
         
        tarnsferRequests[id].to.transfer(transferRequests[id].amount);
   }
}
So again this function is very bare bones. But gives you and idea of how to handle approvals We first make sure that the peron calling the function has not alread approved this transfer. If this is met then we set their approval mapping to true to prevent them from approving again. Once we do this then we increase the approavls atribute for the transfer we are apporving by 1. Then we check does the number of approvals of the transfr id we pass in equal the appoval limit. if so then execute the transfer, if not then do nothing.

Again this code is very bare bone sand you should add a lot more. This code also assumes yu are not going to delete transfers from the array once they are approved. If yu wanted to achieve this then you would need to use a for loop to check if the trasnfer your trying to approve exists. its more complex so i will not go into ut here. Try get this working and come back to me if you would like to do the latter.

1 Like

so many thanks man!!! i’m going to read it 3 more times but i get it i had to put the double mapping in the approval struct :slight_smile: and then write it myself to make sure i can do it myself. there’s one thing i don’t understand about the contract but first i will adjust my approval!

1 Like

perfect let me know how you get on

Alright guys, Hello to everyone. I’ve started maybe a week ago, watching only the introduction video, programming a wallet with fixed 3 owners than, after I was finished I realized that this wasn’t the request, lol ahah. :rofl:
So here I am after have watched also the assistance with a code that maybe can be optimized, surely about with less variables. I realized that I don’t really know how to use the return command for helping me in creating a more smooth code (so I don’t need the abicode v2). :sweat_smile:
Another thing, I was going crazy with the "if in the onlyOwners contract, I searched on the web and I find the solution but I don’t understand why I have to put return after _; and revert; in the end.
Well, I finished my boring speech. Here is the code. I think is fine.

OnlyOwners.sol

pragma solidity 0.7.5;

contract Ownable{

    address[] public owners;

    modifier OnlyOwners {
        for(uint i=0; i<owners.length; i++){
            if(msg.sender == owners[i]){ _; return;} else{continue;}
        }
        revert();
    }
}'''

Wallet.sol

pragma solidity 0.7.5;

import "./OnlyOwners.sol";

contract Wallet is Ownable{

    constructor(uint limit, uint vote_required){
        limit_owners = limit;
        minimum_vote = vote_required;
    }
    uint limit_owners;
    uint minimum_vote;

    transfer[] public pending_transaction;

    uint i = 0;

    uint public wallet_balance;
    mapping(address => mapping(uint => bool)) internal transfer_votes;

    struct transfer{
        uint amount;
        address payable receiver;
        uint vote_done;
        bool hasbeensent;
        uint id;
    }

    function declare_owners(address owner) public {
        if(i<limit_owners){
        owners.push(owner);
        i++;
        } else{revert("Full");}
    }

    function deposit() public payable{
        wallet_balance += msg.value;
    }

    function create_transfer_request(uint _amount, address payable _to) public OnlyOwners {
        require(_amount <= wallet_balance, "Not enough balance");
        require(_to != address(this), "Can't re-send to the contract dude");
        pending_transaction.push(
            transfer(_amount, _to, 1, false, pending_transaction.length)
            );
        transfer_votes[msg.sender][pending_transaction.length] = true;
    }

    function accept(uint index, bool vote) public OnlyOwners{
        require(transfer_votes[msg.sender][index] == false, "You have already voted");
        transfer_votes[msg.sender][index] = vote;
        pending_transaction[index].vote_done ++;        
        transfer_exe(index);
        }

    function transfer_exe(uint ind) internal {
        require(pending_transaction[ind].amount <= wallet_balance, "Not enough balance");
        uint true_count = 0;
        uint false_count = 0;
            if(pending_transaction[ind].vote_done == owners.length){
                for(uint j = 0; j<owners.length; j++){

                    if(transfer_votes[msg.sender][ind]== true){true_count ++;} else{false_count++;}
                }

            if(true_count >= minimum_vote){
                pending_transaction[ind].receiver.transfer(pending_transaction[ind].amount);
                wallet_balance -= pending_transaction[ind].amount;
                pending_transaction[ind].hasbeensent = true;
                }
            }
        }
}
1 Like

[UPDATE - Issue Resolved]

I think I found the problem, working on a solution now…
The Solidity Compiler had a hidden error that didn’t show up in the editor:

UnimplementedFeatureError: Copying of type struct 
MultiSigWallet.Signature memory[] memory to storage 
not yet supported.

I then had to hunt for this and found the culprit in my CreateRequest() function:

Signature[] memory signatures; // <-- this line

I cannot store a memory variable type to a storage data location; unfortunately it’s not an easy "replace memory with storage" solution.

I have to change:

        uint256 requestID = TransferRequests.length;
        Signature[] memory signatures;

        TransferRequests.push(
            Request(
                requestID,
                _getOwner(),
                _amount,
                _toAddress,
                _pendingStatus,
                signatures
            )
        );

To:

        TransferRequests.push();
        uint256 requestID = TransferRequests.length - 1;
        Request storage newRequest = TransferRequests[requestID];
        newRequest.ID = requestID;
        newRequest.Owner = _getOwner();
        newRequest.Amount = _amount;
        newRequest.ToAddress = _toAddress;
        newRequest.Status = _pendingStatus;

This source is what helped me find a solution to this problem.

I can now Compile my MultiSigWallet Contract and debug/fix my project.


Help Please - Remix won’t let me deploy

I’m not able to get Remix to work with my 3 files, I don’t know what’s wrong but the main multisig_wallet.sol file isn’t showing up in the Contract dropdown menu in order for me to Deploy it; however my other 2 files (owner.sol and transfer_request.sol) show up. I don’t see any errors, can anyone here see errors in my code that would cause this? Or test to see if this same problem happens to you with my code?

What I did:

I coded this all offline in Visual Studio Code first (with a Solidity linter/formatter); then in Remix I created each file and copied all the contents in. Perhaps there’s something wrong with doing this?

What I’ve tried:

If I comment almost everything out in multisig_wallet.sol, it’ll finally show up in the Deploy dropdown but when I Deploy it (even after un-commenting all public functions and removing errors) it won’t show the Contracts public functions for me to click in the Deployed Contracts sections. Eventually multisig_wallet.sol will disappear again from the Deploy dropdown, I have no idea what’s going on :S this seems like a glitch with Remix, but it’s possible there’s something else wrong that Remix isn’t able to lint?

I’ve tried adding public pure functions, which didn’t work.

I tried removing the :no_good_man: emoji in case that is breaking the compiler, nope…


my files on Gist


My files:

owner.sol:

pragma solidity 0.8.7;

contract Owner {
    //// TYPE DECLARATIONS ////
    // inspired by:
    // https://ethereum.stackexchange.com/a/12539/86218
    // https://stackoverflow.com/a/49637782/1180523
    struct User {
        address Address;
        bool Exists;
    }

    //// STATE VARIABLES ////
    mapping(address => User) internal OwnerList;

    //// EVENTS ////

    //// MODIFIERS ////

    // isOwner requires that msg.sender is in the OwnerList.
    modifier isOwner() {
        require(
            !OwnerList[msg.sender].Exists,
            "You must be an owner to do this"
        );
        _;
    }

    //// PUBLIC FUNCTIONS ////

    //// PRIVATE FUNCTIONS ////
}

transfer_request.sol:

pragma solidity 0.8.7;

import "./owner.sol";

contract TransferRequest is Owner {
    //// TYPE DECLARATIONS ////
    struct Signature {
        User Owner;
        bool Approved;
    }
    struct Request {
        uint256 ID;
        User Owner;
        uint256 Amount;
        address payable ToAddress;
        string Status;
        Signature[] Signatures;
    }
    struct RequestsLookup {
        uint256[] Pending;
        uint256[] Completed;
        // TODO: can add more logic to account for Transfer Requests that get
        // denied. (chris)
        // uint256[] Denied;
    }

    //// STATE VARIABLES ////
    uint256 internal RequiredApprovals;
    Request[] internal TransferRequests;
    RequestsLookup internal RequestLog;
    string _pendingStatus = "pending";
    string _completedStatus = "completed";

    //// EVENTS ////

    //// MODIFIERS ////

    // notSelfSigning requires that msg.sender isn't the Request.Owner.
    modifier notSelfSigning(uint256 _requestID) {
        require(
            msg.sender != TransferRequests[_requestID].Owner.Address,
            "Cannot self-sign Transfer Requests"
        );
        _;
    }

    //// PUBLIC FUNCTIONS ////

    //// PRIVATE FUNCTIONS ////
}

multisig_wallet.sol:

pragma solidity 0.8.7;

import "./transfer_request.sol";

contract MMultiSigWallet is TransferRequest {
    //// TYPE DECLARATIONS ////

    //// STATE VARIABLES ////

    // constructor sets the owner address in OwnerList.
    constructor(address[] memory _owners, uint8 _approvalCount) {
        RequiredApprovals = _approvalCount;
        // Loop through all addresses to add to OwnerList:
        for (uint256 i = 0; i < _owners.length; i++) {
            OwnerList[_owners[i]] = User(_owners[i], true);
        }
    }

    //// EVENTS ////
    event requestCreated(
        uint256 _requestID,
        uint256 _amount,
        User indexed _owner,
        address indexed _to
    );
    event requestApproved(uint256 _requestID, address indexed _signedBy);
    event requestCompleted(uint256 _requestID);

    //// MODIFIERS ////

    //// PUBLIC FUNCTIONS ////

    // CreateRequest will push a new Request to the TransferRequests array if
    // msg.sender is an Owner, and if _amount is greater than 0 and less than or
    // equal to the contracts balance.
    function CreateRequest(uint256 _amount, address payable _toAddress) public isOwner {
        require(_amount > 0, "Amount must be greater than 0");
        require(
            address(this).balance >= _amount,
            "Amount must be <= contract balance"
        );

        uint256 requestID = TransferRequests.length;
        Signature[] memory signatures;

        TransferRequests.push(
            Request(
                requestID,
                _getOwner(),
                _amount,
                _toAddress,
                _pendingStatus,
                signatures
            )
        );
        RequestLog.Pending.push(requestID);
        emit requestCreated(requestID, _amount, _getOwner(), _toAddress);
    }

    // PendingRequests returns all un-completed Transfer Requests if msg.sender
    // is an Owner.
    function PendingRequests() public view isOwner returns (Request[] memory) {
        Request[] memory requests;
        // Loop through the Pending request ID's and return each Transfer
        // Request for the ID's found:
        for (uint256 i = 0; i < RequestLog.Pending.length; i++) {
            uint256 requestID = RequestLog.Pending[i];
            Request memory r = TransferRequests[requestID];
            // NOTE: ".push()" cannot be used due for memory arrays:
            // - Member "push" is not available in struct Request memory[]
            //   memory outside of storage.
            //   docs info: https://docs.soliditylang.org/en/v0.8.10/types.html#allocating-memory-arrays
            // BUT chaning requests to use storage also doesn't work:
            // - This variable is of storage pointer type and can be accessed
            //   without prior assignment, which would lead to undefined behaviour.
            //
            // This is a workaround, I'm not sure if there's a better approach:
            // requests.push(r); // 🙅 NOPE!
            requests[i] = r;
        }
        return requests;
    }

    // CompletedRequests returns all completed Transfer Requests, this is
    // available to Owners and non-Owners.
    function CompletedRequests() public view returns (Request[] memory) {
        Request[] memory requests;
        // Loop through the Completed request ID's and return each Transfer
        // Request for the ID's found:
        for (uint256 i = 0; i < RequestLog.Completed.length; i++) {
            uint256 requestID = RequestLog.Completed[i];
            Request memory r = TransferRequests[requestID];
            // NOTE: ".push()" cannot be used, see comment in PendingRequests().
            requests[i] = r;
        }
        return requests;
    }

    // ApproveRequest adds an Approved signature to a Request if isOwner and not
    // self-signing the Request; Then if all required approvals are met, the
    // Request is moved from Pending to Completed status and the funds are
    // transferred.
    function ApproveRequest(uint256 _requestID)
        public
        payable
        isOwner
        notSelfSigning(_requestID)
    {
        Request storage request = TransferRequests[_requestID];
        require(request.Amount != 0, "Transfer Request not found");
        require(request.Amount > address(this).balance, "Insufficient funds");

        request.Signatures.push(Signature(_getOwner(), true));
        emit requestApproved(_requestID, _getOwner().Address);

        // if we have enough signatures, mark the request as complete and
        // transfer the funds:
        if (request.Signatures.length >= RequiredApprovals) {
            _pendingToCompleted(request);
            request.ToAddress.transfer(request.Amount);
            emit requestCompleted(_requestID);
        }
    }

    //// PRIVATE FUNCTIONS ////

    // _getOwner returns msg.sender as a User{} struct type if the msg.sender is
    // an Owner.
    function _getOwner() private view isOwner returns (User memory) {
        return User(msg.sender, true);
    }

    // _pendingToCompleted moves the _request.ID out of RequestLog.Pending to
    // RequestLog.Completed and marks _request.Status as completed.
    function _pendingToCompleted(Request storage _request) internal isOwner {
        uint256 pendingLength = RequestLog.Pending.length;
        uint256 completedLength = RequestLog.Completed.length;
        string memory previousStatus = _request.Status;
        uint256[] memory newPending;

        // Move requestID out of Pending to Completed TransferRequests:
        uint256 j = 0;
        for (uint256 i = 0; i < pendingLength; i++) {
            if (RequestLog.Pending[i] != _request.ID) {
                newPending[j] = RequestLog.Pending[i];
                j++;
            }
        }
        RequestLog.Pending = newPending;
        RequestLog.Completed.push(_request.ID);
        _request.Status = _completedStatus;

        assert(RequestLog.Pending.length == pendingLength - 1);
        assert(RequestLog.Completed.length == completedLength + 1);
        // to compare strings, convert them to bytes, generate a Hashes, and
        // compare the hashes:
        // https://docs.soliditylang.org/en/v0.8.10/types.html#bytes-and-string-as-arrays
        assert(
            keccak256(bytes(_request.Status)) !=
                keccak256(bytes(previousStatus))
        );
    }
}

hi Evan,

you really got me going on the right track. i have it almost working. the only thing is that i don’t understand how this situation works:
lets say:
person a = constructer, he sets the amount of sign needed and the coowners: person b, ander person c. Person a deposits amount X. person b and person c have not deposited any. because i said
function deposit () payable public {
Balance[msg.sender] += msg.value;
}
if there are enough approvals (from people other than A) it will sent the amount. but if person A deposited and person B makes the transfer it will not send the transaction because the contract says this msg.sender has no balance of course. i get that he doesn’t have the balance. but i just cannot understand how i will get the wallet to be of all the owners. i assume you cannot say balance(owner2) += msg. value and the same for c because then it would seem there is three time the amount of money there… or … mmm not if you decrease the amount of all owners balances… i don’t know am i on the right track or does it work differently? should i just go and watch the next video with the full contract? i have everything working as long as the person who deposited = person who made the transaction.

1 Like
pragma solidity 0.7.5;
pragma abicoder v2;

contract MultiSigWallet {

    // deposits made by various addresses
    mapping(address => uint) deposits;
    
    // Array list of owners addresses
    address[] owners;
    uint approval_limit; // Limit of approvals before request is approved

    struct Request {
        uint id;
        address payable requested_by;
        uint amount;
        uint approved_num;
        bool withdrawn;
    }

    mapping(uint => mapping(address => bool)) _approvals;
    mapping(uint => Request) requests;

    modifier requestExists(uint id) {
        require(requests[id].amount > 0, "Request does not exists!");
        _;
    }

    modifier canApprove() {
        bool isOwner = false;
        for (uint i=0; i < owners.length; i++) {
            if(owners[i] == msg.sender) {
                isOwner = true;
            }
        }
        require(isOwner, "Must be owner to approve withdraw!");
        _;
    }

    event TransferRequest(uint request_id, address requested_by, uint transfer_amount);
    event RequestApproval(uint request_id, address approved_by);
    event RequestApprovedAndWithdrawn(uint request_id);

    constructor (address[] memory _owners, uint _approval_limit) {
        owners = _owners;
        approval_limit = _approval_limit;
    }

    // Get Owners of MultiSig
    function getOwners() public view returns (address[] memory){
        return owners;
    }

    // deposit ether to address
    function deposit() public payable {
        deposits[msg.sender] += msg.value;
    }

    function getContractBalance() public view returns (uint) {
        return address(this).balance;
    }

    // Get Request details
    function getRequest(uint id) public view requestExists(id) returns (Request memory) {
        return requests[id];
    }

    // Create new Withdraw Request
    function requestWithdraw(uint id, uint amount) public canApprove returns (uint){
        require(address(this).balance >= amount, "Cannot withdraw more than balance!");
        require(amount > 0, "Cannot withdraw zero!");

        Request memory newRequest = Request({
            id: id,
            requested_by: payable(msg.sender), 
            amount: amount, 
            approved_num: 0, 
            withdrawn: false
        });

        requests[id] = newRequest;
        emit TransferRequest(id, msg.sender, amount);

        return id;
    }

    // Approve Request for withdrawl
    function approveRequest(uint id) public canApprove requestExists(id) {
        require(requests[id].requested_by != msg.sender, "Cannot self approve!");
        require(_approvals[id][msg.sender] == false, "Already approved!");
        require(requests[id].withdrawn == false, "Already approved & withdrawn!");
        
        _approvals[id][msg.sender] = true;

        requests[id].approved_num += 1;

        emit RequestApproval(id, msg.sender);

        if(requests[id].approved_num >= approval_limit) {
            requests[id].withdrawn = true;
            (requests[id].requested_by).transfer(requests[id].amount); // Amount sent to requesters' wallet
            emit RequestApprovedAndWithdrawn(id);
        }
    }

}

Hey @minke. Ok so you are on the right track dont worry. Depositing has nothing to do whether the transfer goes throuh or not. Iy ou create the contract as person A and add person B and person C, they dont need to have a balance to be able to send transfers. The only person who needs a balance is the person who initially created the transfer. Also msg.sender changes. msg.sender will not always equal personA in this case bur rather msg.sender will alwsy be the address of the person who is calling the function. Im not sure if you knew this but just wanted to make it crystal clear

Here is my attempt, please review.
I believe everything works.

// SPDX-License-Identifier: MIT

pragma solidity 0.8.10;

contract MultiOwned {
    // This could be a huge list, so considered not gas-safe to iterate.
    address[] private owners;
    
    // This mapping allows cheap checks of the owners list.
    mapping(address => bool) private ownerLookup;

    constructor(address[] memory _owners) {
        owners = _owners;
        for(uint i = 0; i < _owners.length; i++) {
            ownerLookup[_owners[i]] = true;
        }
    }

    modifier onlyOwners {
        require(ownerLookup[msg.sender] == true, "Owners only.");
        _;
    }

    /// @notice Get a list of owners who can create and approve requests.
    /// @dev Should not be used by contracts, unbounded array access.
    function getOwners() view external returns (address[] memory){
        return owners;
    }
}
// SPDX-License-Identifier: MIT

pragma solidity 0.8.10;

import "./MultiOwned.sol";

contract MultiSigFast is MultiOwned {
    
    // For sotring state of request.
    struct Request {
        address payable recipient;
        uint amount;
        // Has the amount been sent.
        bool complete;
        // Only used to remove contract from openRequest list, value is invalid when complete.
        uint openRequestIndex;
        // For cheaply checking if an specific owner has approved.
        mapping(address=>bool) hasApproved;
        // For counting approvers.
        uint approvals;
    }

    // Returned from approve() so user can see if they completed the request.
    struct RequestStatus {
        uint approvals;
        bool complete; 
    }
    
    // For view
    struct RequestInfo {
        uint amount;
        uint approvals;
        bool complete;
        address recipient;  
    }

    uint private minApprovals;
    // Every request gets a unique Id to refer to it by.
    uint private nextRequestId = 0;
    // A list of incomplete requests. There is some cost to maintaining this list but makes veiwing and counting open requests cheaper.  
    uint[] private openRequests;
    // The actual requests.
    mapping(uint=>Request) private requests;
    
    /// @dev Triggered when final approval is made and the contract pays out.
    event Transferred (uint requestId, address recipient, uint amount);

    /// @notice Create wallet with specified list of owners and a minimum number requires to approve a withdrawl.
    constructor (uint _minApprovals, address[] memory _owners) MultiOwned(_owners) {
        minApprovals = _minApprovals;
    }

    receive() external payable{
    } 

    fallback() external payable {
    }

    function getBalance() external view returns(uint) {
        return address(this).balance;
    }

    /// @notice Get the request Ids of any not yet approved requests.
    /// @dev Not intended for use in transactions, unbounded array access.
    function getOpenRequestIds() external view returns(uint[] memory) {
        return openRequests;
    }
    
    /// @notice Get full info on a particular request.
    /// @dev Not intended for use in transactions, unbounded array access.
    function getRequestInfo(uint _requestId) external view returns(RequestInfo memory) {
        Request storage r = requests[_requestId];
        return RequestInfo(r.amount, r.approvals, r.complete, r.recipient);
    }

    function isApprover(uint _requestId, address _approver) public view returns (bool) {
        return requests[_requestId].hasApproved[_approver];
    }

    /// @notice Creates a new request with you as the first approver.
    function createRequest(address payable _recipient, uint _amount) external onlyOwners returns (uint) {
        uint requestId = nextRequestId;
        nextRequestId++;

        openRequests.push(requestId);

        // Structs with mappings cannot exist in memory hence the awkward way of creating entries.
        Request storage r = requests[requestId];
        r.recipient = _recipient;
        r.amount = _amount;
        r.complete = false;
        r.openRequestIndex = openRequests.length -1;
        r.hasApproved[msg.sender] = true;
        r.approvals = 1;

        return requestId;
    }

    /// @notice Adds your approval to request and executes trasnaction if minimum approvers reached. Will need to re-approve if transaction fails for any reason.
    function approve(uint _requestId) external onlyOwners returns(RequestStatus memory) {
        require(_requestId < nextRequestId, "Invalid request ID.");
        Request storage r = requests[_requestId];
        require(r.complete == false, "Request already completed.");
        require(r.hasApproved[msg.sender] == false, "Already approved by you.");
        r.hasApproved[msg.sender] = true;
        r.approvals ++;

        if(r.approvals >= minApprovals) 
        {
            // Checks
            assert(r.complete == false);
            require(address(this).balance > r.amount, "Insufficent contract balance, fund then re-approve.");

            // Effects
            r.complete = true;
            removeOpenRequest(r.openRequestIndex);            
            emit Transferred(_requestId, r.recipient, r.amount);

            // Interactions
            (bool success, ) = (r.recipient).call{value:r.amount}("");
            require(success, "Transaction failed, approval rolled back, try re-approving?");    
        }

        return RequestStatus(r.approvals, r.complete);
    }

    /// @dev Remove item and shrink array by overwriting item with last item in array then popping the last item. 
    function removeOpenRequest(uint _index) private {
        // TODO: Figure out how much gas this burns.
        if(openRequests.length - 1 == _index)
        {
            openRequests.pop();
        }
        else
        {
            uint lastItem = openRequests[openRequests.length - 1];
            openRequests[_index] = lastItem;
            openRequests.pop();
            requests[lastItem].openRequestIndex = _index;
        }
    }
}

Unbounded arrays

I wanted to make my solution scale for use with huge numbers of approvers, requests and approvals. Not sure if my constructor can handle that but createRequest() and approve() were designed to scale.

  • I avoided array iteration in the createRequest() and approve() code.
  • I think array iteration was unavoidable in the constructor without making the owners mutable. Managed to crash Remix by passing 100 owners in to it.
  • The only functions that deal with entire arrays are the external views, which are not intended to be called in transactions, so gas doesn’t matter

Partial Reverts?

I couldn’t think of a simple way to not revert the approval when the transaction fails. I think approve and send would need to be separated.

Gas efficiency

Request used to have an array of approvers, which was handy for viewing who has approved that request so far. By replacing that with just a uint count of approvals I saved about 20% gas on createRequest and approve. However now it takes a lot of requests to isApprover figure out who the approvers are. Maybe an approve event would help here?

Could save more gas by scrapping the openRequests array, but than a user or DAPP would have to getEveryRequestEver() and then isRequestComplete(uint id) on each one. Is the event log suited to this?

Alright, here’s my submission after much personal trial-and-error without looking at any of the assistance or solution videos… this is all from scratch and I learned A LOT doing this all on my own after about 9’ish hours working on this.

This was a great learning experience, and I hope to learn more with any feedback on how I can improve my code. You can see my first draft in this post where I got stuck with a compiler error :laughing: (first draft), you’ll notice a LOT has changed, while lots of it is still very similar or identical.


ownable.sol:

pragma solidity 0.8.7;

contract Ownable {
    //// TYPE DECLARATIONS ////
    struct User {
        address Address;
        bool Exists;
    }

    //// STATE VARIABLES ////
    mapping(address => User) internal ownerList;

    //// EVENTS ////

    //// MODIFIERS ////

    // isOwner requires that msg.sender is in the OwnerList.
    modifier isOwner() {
        // inspired by:
        // https://ethereum.stackexchange.com/a/12539/86218
        // https://stackoverflow.com/a/49637782/1180523
        require(
            ownerList[msg.sender].Exists,
            "You must be an owner to do this"
        );
        _;
    }

    //// PUBLIC FUNCTIONS ////

    //// PRIVATE FUNCTIONS ////
}

multisig_wallet.sol:

pragma solidity 0.8.7;

import "./ownable.sol";

contract MultiSigWallet is Ownable {
    //// TYPE DECLARATIONS ////
    struct Signature {
        User Owner;
        bool Signed;
    }
    struct Request {
        uint256 ID;
        User Owner;
        uint256 Amount;
        address ToAddress;
        string Status;
        Signature[] Signatures;
    }
    struct RequestsLookup {
        uint256[] Pending;
        uint256[] Completed;
        // TODO: can add more logic to account for Transfer Requests that get
        // denied. (chris)
        // uint256[] Denied;
    }

    //// STATE VARIABLES ////
    uint256 public requiredApprovals;
    Request[] private transferRequests;
    RequestsLookup private requestLog;
    string private _pendingStatus = "pending";
    string private _completedStatus = "completed";

    // constructor sets the owner address in OwnerList.
    constructor(address[] memory _owners, uint8 _approvalCount) {
        requiredApprovals = _approvalCount;
        // Loop through all addresses to add to OwnerList:
        for (uint256 i = 0; i < _owners.length; i++) {
            ownerList[_owners[i]] = User(_owners[i], true);
        }
    }

    //// EVENTS ////
    event Deposited(uint256 _amount, address indexed _fromAddress);
    event RequestCreated(
        uint256 _requestID,
        uint256 _amount,
        User indexed _owner,
        address indexed _toAddress
    );
    event RequestApproved(uint256 _requestID, address indexed _signedBy);
    event RequestCompleted(uint256 _requestID);

    //// MODIFIERS ////

    // canApproveRequest requests that the Transfer Request was found; that the
    // Contract has enough balance for the Transfer; that the signer isn't
    // trying to self-sign their own Transfer Request; and that the signer
    // hasn't already signed the Transfer Request.
    modifier canApproveRequest(uint256 _requestID) {
        Request memory request = transferRequests[_requestID];
        require(request.Amount != 0, "Transfer Request not found");
        require(request.Amount >= address(this).balance, "Insufficient funds");
        require(
            msg.sender != request.Owner.Address,
            "Cannot self-sign Transfer Requests"
        );
        bool alreadySigned = false;
        for (uint256 i = 0; i < request.Signatures.length; i++) {
            if (msg.sender == request.Signatures[i].Owner.Address) {
                alreadySigned = true;
                break; // exit the loop early
            }
        }
        require(!alreadySigned, "Already signed this Transfer Request");
        _;
    }

    //// PUBLIC FUNCTIONS ////

    // getBalance returns the Contracts balance.
    function getBalance() public view returns (uint256) {
        return address(this).balance;
    }

    // deposit transfers funds to the Contract (anyone can deposit funds).
    function deposit() public payable {
        emit Deposited(msg.value, msg.sender);
    }

    // createRequest will push a new Request to the TransferRequests array if
    // msg.sender is an Owner, and if _amount is greater than 0 and less than or
    // equal to the contracts balance.
    function createRequest(uint256 _amount, address _toAddress) public isOwner {
        require(_amount > 0, "Amount must be greater than 0");
        require(
            address(this).balance >= _amount,
            "Amount must be <= contract balance"
        );

        // NOTE: cannot copy a "memory" type Signature[] to a new Request that
        // is stored in "storage", see error:
        // - UnimplementedFeatureError: Copying of type struct
        //   MultiSigWallet.Signature memory[] memory to storage not yet
        //   supported.
        // uint256 requestID = transferRequests.length;
        // Signature[] memory signatures;
        // transferRequests.push(
        //     Request(
        //         requestID,
        //         _getOwner(),
        //         _amount,
        //         _toAddress,
        //         _pendingStatus,
        //         signatures
        //     )
        // );
        //
        // Here's a workaround for this problem:
        // source: https://github.com/ethereum/solidity/issues/4115#issuecomment-612309066
        transferRequests.push();
        uint256 requestID = transferRequests.length - 1;
        Request storage newRequest = transferRequests[requestID];
        newRequest.ID = requestID;
        newRequest.Owner = _getOwner();
        newRequest.Amount = _amount;
        newRequest.ToAddress = _toAddress;
        newRequest.Status = _pendingStatus;

        requestLog.Pending.push(requestID);
        emit RequestCreated(requestID, _amount, _getOwner(), _toAddress);
    }

    // pendingRequests returns all un-completed Transfer Requests if msg.sender
    // is an Owner.
    function pendingRequests() public view isOwner returns (Request[] memory) {
        // NOTE: unable to do "requests[0] = request;" in loop unless we set a
        // length.
        // solution source: https://stackoverflow.com/a/55532934/1180523
        Request[] memory requests = new Request[](requestLog.Pending.length);
        // Loop through the Pending request ID's and return each Transfer
        // Request for the ID's found:
        for (uint256 i = 0; i < requestLog.Pending.length; i++) {
            // NOTE: ".push()" cannot be used due for memory arrays:
            // - Member "push" is not available in struct Request memory[]
            //   memory outside of storage.
            //   docs info: https://docs.soliditylang.org/en/v0.8.10/types.html#allocating-memory-arrays
            // BUT chaning requests to use storage also doesn't work:
            // - This variable is of storage pointer type and can be accessed
            //   without prior assignment, which would lead to undefined
            //   behaviour.
            //
            // This is a workaround, I'm not sure if there's a better approach:
            // requests.push(r);
            uint256 requestID = requestLog.Pending[i];
            requests[i] = transferRequests[requestID];
        }
        return requests;
    }

    // completedRequests returns all completed Transfer Requests, this is
    // available to Owners and non-Owners.
    function completedRequests() public view returns (Request[] memory) {
        // NOTE: unable to do "requests[0] = request;" in loop unless we set a
        // length.
        // solution source: https://stackoverflow.com/a/55532934/1180523
        Request[] memory requests = new Request[](requestLog.Completed.length);
        // Loop through the Completed request ID's and return each Transfer
        // Request for the ID's found:
        for (uint256 i = 0; i < requestLog.Completed.length; i++) {
            uint256 requestID = requestLog.Completed[i];
            requests[i] = transferRequests[requestID];
        }
        return requests;
    }

    // approveRequest adds an Approved signature to a Request if isOwner and not
    // self-signing the Request; Then if all required approvals are met, the
    // Request is moved from Pending to Completed status and the funds are
    // transferred.
    function approveRequest(uint256 _requestID)
        public
        isOwner
        canApproveRequest(_requestID)
    {
        Request storage request = transferRequests[_requestID];
        request.Signatures.push(Signature(_getOwner(), true));
        emit RequestApproved(_requestID, _getOwner().Address);

        // if we have enough signatures, mark the request as complete and
        // transfer the funds:
        if (request.Signatures.length >= requiredApprovals) {
            _pendingToCompleted(request);
            payable(request.ToAddress).transfer(request.Amount);
            emit RequestCompleted(_requestID);
        }
    }

    //// PRIVATE FUNCTIONS ////

    // _getOwner returns msg.sender as a User{} struct type if the msg.sender is
    // an Owner.
    function _getOwner() private view isOwner returns (User memory) {
        return User(msg.sender, true);
    }

    // _pendingToCompleted moves the _request.ID out of RequestLog.Pending to
    // RequestLog.Completed and marks _request.Status as completed.
    function _pendingToCompleted(Request storage _request) private isOwner {
        uint256 pendingLength = requestLog.Pending.length;
        uint256 completedLength = requestLog.Completed.length;
        string memory previousStatus = _request.Status;
        uint256[] memory newPending;

        // Move requestID out of Pending to Completed TransferRequests:
        uint256 j = 0;
        for (uint256 i = 0; i < pendingLength; i++) {
            if (requestLog.Pending[i] != _request.ID) {
                newPending[j] = requestLog.Pending[i];
                j++;
            }
        }
        requestLog.Pending = newPending;
        requestLog.Completed.push(_request.ID);
        _request.Status = _completedStatus;

        assert(requestLog.Pending.length == pendingLength - 1);
        assert(requestLog.Completed.length == completedLength + 1);
        // to compare strings, convert them to bytes, generate a Hashes, and
        // compare the hashes:
        // https://docs.soliditylang.org/en/v0.8.10/types.html#bytes-and-string-as-arrays
        assert(
            keccak256(bytes(_request.Status)) !=
                keccak256(bytes(previousStatus))
        );
    }
}

1 Like

Hi @filip , i have a question about the multisig wallet. when you say in project introduction video Anyone should be able to deposit ether into the smart contract, does it mean anyone of the owners or any account in the blockchains ? but i guess it is anyone of the owners.

1 Like

yes any of the owners

hi, thanks @mcgrane5.

1 Like

very cool solution @skplunkerin. on your createRequests function. I see that your are tracking user signatures via a signature struct and this is preventing you from doing the one liner to instiate the struct and puhs the instance to transfer requests. If you change the signature struct to a double mapping

mapping(address => mapping(uint => bool) approvals

and change the signatures attrbute in your struct to a uint numOfApprovals. then you can effectively use this one liner method which is much more clean and you still are able to keep track of information on which account approved which transfer trhough the mapping. whats more is you can emit and event called transferApproved which you can capute even more info such as the approver, the transferid the time of approval etc. although you acc have the approve event so thats fine. Big tip you will begin to see that when you start to develop UI’s you can get away with making a lot of data structures by just emmiting events instead and fetching the event in your application.

Note that i mention this not to critique your code because it is well and trully very impressive but moreso to address the issue that you highlighed in your commented out part. Hats off though great stuff

1 Like