Project - Multisig Wallet

[szaslavsky]

@mcgrane5 - I feel that the approvals should go inside the TransferRequest struct because they are very relevant to the state of the TransferRequest. For the initial version of the assignment I was not able to accomplish this because a mapping cannot exist inside a struct in storage so I had to resort to a hack by storing the approvals in a separate storage array. The array indexes of the requests array and the approvals array lined up 1:1 so I got away with it.

For the second version (presented below) I determined that an array can be part of a struct in storage but it seems a bit tricky to initialize such a struct - see my comment in createTransferRequest().

pragma solidity 0.7.5;

import “./Ownable.sol”;

contract MultiSigWallet is Ownable
{
enum RequestState {UNKNOWN, PENDING, SENT, CANCELLED }

struct TransferRequest{
    uint id;
    uint amount;
    address destination;
    RequestState state;
    uint requiredNumberOfApprovals;
    uint currentNumberOfApprovals;
    address[] addressesOfApproversThatHaventApproved;
 }

TransferRequest[] transferRequests;

uint defaultRequiredNumberOfApprovals;
// this is a list of addresses that is used as a template for valid approvers when creating new transferRequests
address[] defaultApprovalAddresses;

event transferred(uint requestid, uint amount, address addr);
event approvedBySignatory(uint requestid, address addr, uint currentNumberOfApprovals);

function setRequiredNumberOfApprovals(uint _numApprovals) public onlyOwner
{
    defaultRequiredNumberOfApprovals = _numApprovals;
}

function getRequiredNumberOfApprovals() public view onlyOwner returns(uint) 
{
    return defaultRequiredNumberOfApprovals;
}

function setApprovalAddresses( address[] memory _addresses) public onlyOwner
{
    defaultApprovalAddresses = _addresses;
}

function getApprovalAddresses() public view onlyOwner returns(address[] memory)  
{
    return defaultApprovalAddresses;
}

// attempts to cancel the request while it is in PENDING state and returns true if the request is in cancelled state at the end of the operation
function cancelTransferRequest(uint _requestId) public onlyOwner returns(bool)  
{
    TransferRequest storage theRequest = transferRequests[_requestId];
    
    if( theRequest.state == RequestState.PENDING )
    {
        theRequest.state = RequestState.CANCELLED;
    }
    
    return theRequest.state == RequestState.CANCELLED;
}

// I want this to return the new balance
function deposit() public payable returns (uint) 
{
    // I wonder if this returns the balance before or after the deposit is complete...
    return address(this).balance;
}

// returns the requestId of the newly created request
function createTransferRequest(uint _amount, address _destination) public returns (uint)
{
    uint newRequestId = transferRequests.length;
    
    // Create the request in memory, then move to storage because
    // when I create it directly in storage, it wants me to 
    // initialize addressesOfApproversThatHaventApproved but 
    // I can't figure out how to create an empty address[] on storage
    TransferRequest memory newRequest;
    
    transferRequests.push( newRequest );

    TransferRequest storage newRequestStorage = transferRequests[newRequestId];
    newRequestStorage.id = newRequestId;
    newRequestStorage.amount = _amount;
    newRequestStorage.destination = _destination;
    newRequestStorage.state = RequestState.PENDING;
    newRequestStorage.requiredNumberOfApprovals = defaultRequiredNumberOfApprovals;
    newRequestStorage.currentNumberOfApprovals = 0;
    
    // set up all of the addresses that can approve this request in remainingApprovalAddresses
    bool senderIsOneOfDefaultAddresses;
    for(uint i = 0; i < defaultApprovalAddresses.length; i++)
    {
        address thisAddress  = defaultApprovalAddresses[i];
        newRequestStorage.addressesOfApproversThatHaventApproved.push(thisAddress);
        if(thisAddress == msg.sender)
            senderIsOneOfDefaultAddresses = true;
    }
    
    require(senderIsOneOfDefaultAddresses, "This Sender is not allowed to create requests.");
  
    return newRequest.id;
}

// notes the approval and sends the amount if the required number of approvals is reached
// returns the state of the request after the function is complete
function approveAndSend(uint _requestId) public returns (RequestState)
{
    require(_requestId < transferRequests.length);
    TransferRequest storage theRequest = transferRequests[_requestId];
    
    if( theRequest.state == RequestState.PENDING )
    {
        // it's possible to approve even if there's not enough money in the contract, but it won't be sent and the approval will be rolled back
        for(uint i = 0; i < theRequest.addressesOfApproversThatHaventApproved.length; i++)
        {
            if(theRequest.addressesOfApproversThatHaventApproved[i] == msg.sender)
            {
                // approved by an approver that has not yet approved this request
                theRequest.currentNumberOfApprovals += 1;
                delete theRequest.addressesOfApproversThatHaventApproved[i]; // mark this approver down as having approved this request so that this approver doesn't get to approve multiple times
                emit approvedBySignatory(theRequest.id, msg.sender, theRequest.currentNumberOfApprovals);
            }   
        }
    }
    
    if( theRequest.currentNumberOfApprovals >= theRequest.requiredNumberOfApprovals )
    {
        theRequest.state = RequestState.SENT;
        payable(theRequest.destination).transfer(theRequest.amount);
        emit transferred(theRequest.id, theRequest.amount, theRequest.destination);
    }
    
    return theRequest.state;
}

}

[mcgrane5]

hey @szaslavsky. i dont think the approvals need to go intot the strcut. You can think about all of these different data strctures like tools that you can use to manipulaye your code. Its actually a very very common storage design pattern in solidity to have a struct like this and then to create a mapping like so to do various checks like changing boolean states for example. Its also very common to use structs with arrays too lke you have done above by creating an array of trasnfer instances.

So dont feel like you should include the approvals as part of the struct. If you want to do so using the approvals array inside the struct as an attribute please see this code i adapted from one of my previous posts on how to do that

struct Transfer {
        uint amount;
        address destination;
      
        uint requiredNumberOfApprovals;
        uint currentNumberOfApprovals;
        address[] addressesOfApproversThatHaventApproved;
    }

you can then create another mapping called transfrs instead of having a transfer truct array. We will use this to map an id to a tramsfer

mapping(uint => Transfer) transfers

then You will need to change your createTransfer( ) function like so to be able to push addresses to your approvers array inside the struct. Then when we have initilsisd your strict we can assign the new instance to some iD an associate it with our transfer mapping

function createTransfer (address payable recepient, uint amount) public onlyOwner returns (uint){
        require(address(this).balance >= amount, "Insufficient balance in Wallet");
        
        //set id to zero. This is best done globally but i am setting it here just as an example to show you 
         uint id = 0;

        Transfer memory new_transfer = Transfer(

            //fill in all of your other attributes
            //here but initialise your array attribute like i have done below
            new address[](0),
            
        );

       //now we can assign this transfer instance to our transfer mapping (ID = 0)
       transfer[id] = new_transfer;
  
       //increment id for next ransfer
       id++;

        ......
        ......
        .......

conversley in your approve function to check whether an address has alread approved set up your requiement like this

function approveTransfer (address _approver) public  returns (uint){
       
        for (uint i = 0; i < transfers[_id].approvers.length; i++) {
            if(transfers[_id].approvers[i] == msg.sender) revert();
        }

       //then we can push in the address into our approver array in our struct
       transfers[_id].approvers.push(_approvers);

       ........
       ........
       ........

Note that the solutions is how above are only very general to your code i did use your code template but the functions are incomplete you will have to do other things they serve as the bare bones to get you going with this approach. Let me know if you understand correctly

Evan

[1984]

pragma solidity 0.7.5;

pragma abicoder v2;

contract multisigWallet {

address owneraddress1;
address owneraddress2;
address owneraddress3;

constructor (address ad1, address ad2, address ad3){
    owneraddress1=ad1;
    owneraddress2=ad2;
    owneraddress3=ad3;
}

struct transaction {uint _transactionIndex; uint _amount; address _to;}
transaction[] transactionList;

mapping (address=>uint) transactionIndex;
mapping (uint=>uint) transactionAmount;
mapping (uint=>uint) numberOfValidation;
mapping (address=>mapping(uint=>bool)) validationList; 

uint transactionNumber;

function deposit() public payable{}

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

function createTransaction(address to, uint amount) public {
    transactionNumber +=1;
    transactionIndex[to]= transactionNumber;
    transactionAmount[transactionIndex[to]]=amount;
    transaction memory newtransaction = transaction(transactionIndex[to],amount,to);
    transactionList.push(newtransaction);
}

function getTransaction(uint _transactionNumber) public view returns (transaction memory){
    transaction memory transactionToReturn= transactionList[_transactionNumber-1];
    return transactionToReturn;
}

function validateTransaction(bool validation, uint _transactionNumber) public {
    if((msg.sender==owneraddress1||msg.sender==owneraddress2||msg.sender==owneraddress3)
    &&(validationList[msg.sender][_transactionNumber]!=true&&validation == true)){
        numberOfValidation[_transactionNumber]+=1;} else {"you did not validate";}
        validationList[msg.sender][_transactionNumber]= validation;
        if(numberOfValidation[_transactionNumber]==2){
        payable(transactionList[_transactionNumber-1]._to).transfer(transactionList[_transactionNumber-1]._amount);
        }
}

function getStatus (uint _transactionNumber) view public returns (uint) {
    return numberOfValidation[_transactionNumber];}

}

[1984]

Hi there, I have to say that this course was really good, and I am happy that a few months ago I did not know anything about coding and now I managed to write that without looking at the solution Thx @filip

[Random]

Hi @mcgrane5 sure, here it is:

pragma solidity 0.7.5;
import "./Owned.sol"; 

pragma abicoder v2;

 

contract Wallet1 is Owned  {

    address[] owners;

    uint limit;

    uint walletBalance;
    
    
event transactionRequestCreated(address indexed _from, address indexed to, uint indexed _id, uint _value);

event transactionCompleted(address indexed _from, address indexed to, uint indexed _id, uint _value);
    

   

    

    struct Transaction{

        address payable to;

        uint amount;

        uint txID;

    }

   

    Transaction[] transferRequests;

    

    //mapping with two inputs

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

    

    mapping(uint => Transaction) transactions;

    

    constructor(address[] memory owners1, uint approvals1) {

        owners = owners1;

        limit = approvals1;

        

    }

   

    function createTransactionRequest(address payable to, uint amount) public onlyOwner(getNoOfOwners(), owners) {

    

     

      require (getBalance() >= amount, "deposit more ether brudda");

    

     Transaction memory transaction = Transaction(to, amount, transferRequests.length);
     
     

     transferRequests.push(transaction);

     transactions[transaction.txID] = transaction;
     
     emit transactionRequestCreated(msg.sender, transaction.to, transaction.txID, transaction.amount); 

     

 }

    function getTransactionRequests() public view returns (Transaction[] memory) {

        return transferRequests;

 

}

 function approveTransaction(uint txID) public onlyOwner(getNoOfOwners(), owners) {

     

      
        
        uint approved = 0;

         approvals[msg.sender][txID] = true;
        
          
         
         

         for (uint i = 0; i < owners.length; i++){

            

              

             if (approvals[(owners[i])][txID] == true) {

                 approved++;

             }

            

             if (approved >= limit) {

                 //makeTheTransfer

                 transactions[txID].to.transfer(transactions[txID].amount);
                 
                 emit transactionCompleted(msg.sender, transactions[txID].to, transactions[txID].txID, transactions[txID].amount);

                 walletBalance -= transactions[txID].amount;

                 delete transferRequests[txID];

                 approved = 0;

             }

         }

        

     

        

}

   

    function deposit()  public payable {

      

       walletBalance += msg.value;

      

   }

  

   function getBalance() public view returns (uint) {

       return walletBalance;

   }

  

   function getNoOfOwners() public view returns (uint) {

     return owners.length;

}

 function getOwnerAddress(uint index) public view returns (address) {

     return owners[index]; 

     

 }
 
 function getLimit() public view returns(uint) {
     return limit; 
 }
 
 
 
 

   

    

   

    

    

    

    

}

And this is the second file:

pragma solidity 0.7.5;
contract Owned {
    
    

   
    modifier onlyOwner(uint noOfOwners, address[] storage owners) {
        
            
            
            bool msgSenderValid = false; 
            
            for (uint i=0; i<noOfOwners; i++){
                if (msg.sender == owners[i]) {
                    msgSenderValid = true; 
                }
            }
            
            require(
            msgSenderValid == true,
            "Only an owner can call this function.");
            
            
        
        _;
    }
}

Please have a look at it and let me know if you think it is good enough to upload to github. I’m a fairly beginner programmer (even though I’m a software eng undergrad), and I don’t have much on my github so let me know what you think! Maybe I should continue taking the Solidity courses before I start uploading to github? Thanks a bunch!
Ismail

[Random]

Also, I’m a bit confused with the memory/ storage/ calldata. For example when I passed the owners[] array to the onlyOwner() modifier was that passing by reference? Could I have used calldata there as I did not change the contents of owners[] in the function? Thank you!

[apmfree]

Got lazy and hard coded number of owners to 3 and limit to 2…but it works
pragma solidity 0.7.5;
pragma abicoder v2;

// address(this).balance returns total balance of contract

contract Wallet {

//mapping(address => uint) owners;
uint approvalRequired;
address owner1;
address owner2;
address owner3;
struct Transfers {
    address payable to;
    uint trID;
    uint amount;
    bool approved;
    bool completed;
}

Transfers[] Transactions;

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

modifier onlyOwners {
    require(msg.sender == owner1 || msg.sender == owner2 || msg.sender == owner3);
    _; //run the function
}

 constructor(address owner_1,address owner_2, address owner_3) {

    owner1 = owner_1;
    owner2 = owner_2;
    owner3 = owner_3;

    approvalRequired = 2;
}

function deposit() public payable returns (uint) {
    
    return msg.value; 
}

function requestTransfer(address payable payTo,uint amount) public onlyOwners {
    
    
    //recording transaction into array struct
    Transactions.push(Transfers(payTo,Transactions.length,amount,false,false));

}



function approveTransaction(uint TransactionNumber,bool Approve) public payable onlyOwners returns (bool) {
    
        approvals[msg.sender][TransactionNumber] = Approve;
    
        if (isApproved(TransactionNumber) && !Transactions[TransactionNumber].completed){
            
            uint amount = Transactions[TransactionNumber].amount;
            address payable _to = Transactions[TransactionNumber].to;
            
            //making transfer
            
             require(address(this).balance < amount,"Insufficient funds!");
             _to.transfer(amount);
             Transactions[TransactionNumber].completed = true;
             return true;
        }
        else return false;
}


function isApproved(uint num) private returns (bool){
    bool _owner1 = approvals[owner1][num];
    bool _owner2 = approvals[owner2][num];
    bool _owner3 = approvals[owner3][num];
    
    if ((_owner1 && _owner2) || (_owner1 && _owner3) || (_owner2 && _owner3)) {
        Transactions[num].approved = true;
        return true;
    }
    else return false;
    
    
    
}

}

[mcgrane5]

hey @Random. ok so your contract is pretty good. There is a few things you can change in order to make it even better. Right now its a little sparse. for example you should add a withdraw function. Your wallet is missing this. currently as it stands you can deposit funds into the multisig smart contract and do transfers but you are currently unable to withdraw funds from the smart contract which is an issue as this is a key feature for any of the wallet owners to be able to withdraw funds in which they own and have deposited into the wallet. so you could have it as follows

Wait actually, first things first add the following mapping at the top of your contract you will see why shortly

mapping(address => uint) balance

now sorry we can contnue

function withdraw(uint _amount) public onlyOwners returns (uint)
    {
        require(balance[msg.sender] >= _amount);
        balance[msg.sender] -= _amount;
        payable(msg.sender).transfer(_amount)
        
        return balance[msg.sender];
        
    }

this function should be limited to the waallet owners as we dont want people who are not wallet owners to be able to withdraw funds.

Next is your deposit function you should make the following changes. Firstly it should be restricted to onlyOwners as the wallet owners should be the only people who can deposit into the wallet smart contract. Secondly you should add the following require statments to improve the security

function deposit() public payable onlyOwners
    {
        require(msg.value >= 0);
        require(owners.length > 1, "need to have more than one signer");
    
        balance[msg.sender] += msg.value;
    }
    

Notice that i have a balance mapping over your Wallet balance global var. You should change from your variable to this mapping for the reason that you can simple have access to each different acounts balances. Currently as it stands, although i have not tested your code i can say that using a global var like you have presents a big security risk for the following reason. If you were to go to accounts[ 1] and deposit in 1 ether, then account[1] would have 1 ether to choose to with as they please. However if you change to account[2] and deposit andother 1 eth, then the walletBalance variable would now store a value of 2 ether. Thus if you changed back to account[1] and went to call the withdrawal function you would be able to withdraw 2 ether which ishould be impossible

Currently as it stands your walletBalance variable actually represents the contract balance. So if i was you i would actually change its name to “contractBalance”. Its actually handy to be able to keep track of this so what i would suggest actually now that i think on it you should change both the withdraw and deposit functions i described above to be

function withdraw(uint _amount) public onlyOwners returns (uint)
    {
        require(contractBalance >= _amount);
        require(balance[msg.sender] >= _amount);
        balance[msg.sender] -= _amount;
        contractBalance -= _amount;
        payable(msg.sender).transfer(_amount)
        
        return balance[msg.sender];
        
    }

function deposit() public payable onlyOwners
    {
        require(msg.value >= 0);
        require(owners.length > 1, "need to have more than one signer");
    
        balance[msg.sender] += msg.value;
        contractBalance += msg.value;
    }

notice how in the deposit function i increment both the owners balance and also the contract balance itself. And in the withdrawal function decrease the contract balance by the amount so that it is at the correct value.

You should now add a function which returns the users balance and the contract balance. I see that you have a function called getBalance. Just change this to getContractBalance. Then make a new function called getUserBalance. In my opinion the getContractBalance function should be limited with the onlyOner modifier as this can be seen as sensitive infrmation that you dont want anyone to have access to. So your two function should look like

function getContractBalance() public view onlyOwner returns (uint) {

       return getContractBalance;

   }

function getUserBalance() public view onlyOwner returns (uint) {

       return balance[msg.sender];

   }

the next thing i will suggest is a nit picky thing and is not very important. But it makes sense to me that the owners array should ahve more than 1 owner. Meaning you should not de able to deploy the contract unless the owners array has a length larger or equal to 2. Conversley the limit should not be able to be larger than the length of the owners array (otherwise this would break the contract) also it should not be able to be == 0. Now i always proclaim that you should not hardcode the users array in the constructort as it greatly limits the flexibility of your smart contract as if you want to create new owners or delete owners you have to redploy. So you should create a addUser() and deleteUser() function so that you can dynamically add an delet users from the owners array.

I will explain how to do both approaches. If you want to do the constructor method to improve upon what you already have, simply add the changes i mentioned above to your consuctor. So it should look like.

constructor(address[] memory owners1, uint approvals1) {
        require(owners1.length >= 2);
        require(approvals1 != 0 && approvals1 < owners1.length);
        owners = owners1;

        limit = approvals1;
    }

this will improve the security of your SC and prevent people from breaking it. Now i will describe what you should do if you want to take the other (dynamic) approach which i believe you should.

Firstly it makes sense that the account who deploys the wallet contract should be an owner. Secondly by creating an adUser() function we are dynamically creating the owners array, so there is no way to know before hand what our “limit” should be. Thus we will calculate it based off infrmation regardimng the current length of our owners array. When i did this contract i simply just said that limit = owners.length - 1, so the limit would always be 1 - the length of the owners array. I originally wanted to have it be 2/3’s the length but solidity does not handle fixed point arithmetic. There are some librarys out there that i have not fully explored like ABDK Math 64.64 library library which has a divide function. But i have never used this and cannot reccomed using ot as i do not know how it works but if you want to read into it go for it. Ok so with that the code for these changes looks like

//add the contract deployer tp the owner array in the constructor
constructor() {
       owners.push(msg.sender);
    }

function addUsers(address _owners) public OnlyOwners
    {
        for (uint user = 0; user < owners.length; user++)
        {
            require(owners[user] != _owners, "Already registered");
        }
        owners.push(_owners);
        
        //from the current array calculate the value of minimum consensus
        limit = owners.length - 1;
    }

Next is the delete function. This is very simple but the most crucial thing is that you reduce the limit after you delete a user from the array. So your delee user function could look like

function removeUser(address _user) public onlyOwners
    {
        uint user_index;
        for(uint user = 0; user < owners.length; user++)
        {
            if (owners[user] == _user)
            {   
                user_index = user;
                require(owners[user] == _user);
            }
        }
        
        owners[user_index] = owners[owners.length - 1];
        owners.pop();
        limit -= 1;
    }

with these changes your contract is now looking much better. The rest of the changes i mention are extremely nit picky and dont really matter in the grand scheme of things they are just good practice

firstly events. I had F all events in my contract when i did this but the more you code the more you realise events are very good especially when doing front end stuff. So i would make an event for each main function like deposit, wothdraw, approve etc along woth the two you already have

secondly i would add a few morw arguments to your struct like an hasBeenApproved boolean which traacks the current status of the transfer. And also an appprovals attribute which tracks the current number of approvals a transfer has. With this attribute you could then create one more function called getApprovalsForUser or something which returns the current approvals for a specific transfer. TYhis could be handy if you have lodes of transferRequests and forget which is which

function getApprovasForUser(uint _id) public view returns(uint)
    {
        return transferRequests[_id].approvals;
    }

and wth these changes your contract will be even better than what it already is. Congratulations though for finishing this course and finishing the first draft of your contract on your own. I remember i struggled a lot when i was foing this

As for course to do. Definitley solidity 201, also check out react course, and ethereumGame programming course. The game programming course especially.

In my personal opinion i would take the game programming course next nefore solidity 201 for the reason that it will be a major major challenge to finsish with your current knowlege base after completing this course. Tgis challenge will really push you to go out pn your own to find solutions. If you take solidity 201 next and then game programing you will have seen a lot of the topic scovered before in the smartt contract part of it. But the choice is yours, the recommended route would be to take 201 first.

Another course is solidity 201 [old] as this course is very difficult as there is no code htrough at all for the final project, you realy are left on your own. But thats hats so great. Currently 201 [old] is not available on the ivanOnTech site but i will contact carloz for you to see why this is the case so i will get back to you on that.

Anways great job man. See you around the forums.

Evan

[Zaqoy]

Hi, I was completely lost as to how to structure the contract so I peeked ahead to see what functions Filip used. I was able to code the rest on my own, so I consider that an accomplishment! I appreciate any feedback;

pragma solidity 0.7.5;
pragma abicoder v2;

contract MultiSigWallet{

address[] owners;
uint reqApprovals;
uint balance;

//wallet owners and number of required approvals to be set at contract deployment
constructor(address[] memory _owners, uint _reqApprovals){
    owners = _owners;
    reqApprovals = _reqApprovals;
}

modifier onlyOwners {
    bool owner = false;
    for(uint i = 0; i < owners.length; i++){
        if(msg.sender == owners[i]){
            owner = true;
        }
    }
    require(owner == true);
    _; //run the function
}

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

struct Transfer{
    address payable to;
    uint amount;
    uint approvalCount; //keep track of approvals per ID
    bool approved;
    uint transferID;
}

Transfer[] transferLog;

//function to return an array containing wallet owner addresses
function getOwners() public view returns (address[] memory) {
    return owners;
}

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

//function to return the balance of the wallet
function getBalance() public view returns (uint){
    return balance;
}

//function to allow wallet owner to initialize a transfer
function startTransfer(address payable _to, uint _amount) public onlyOwners returns(string memory, uint) {
    uint i = transferLog.length;
    transferLog.push(Transfer(_to, _amount, 0, false, i));
    return ("The ID of this transfer is: ", i);
}

//function to retrieve array containing all initialized transfers
function getTransfers() public view returns(Transfer[] memory) {
    return transferLog;
}

//function to allow wallet owner to approve a specific transfer
function approveTransfer(uint _transferID) public onlyOwners {
    require(approvals[msg.sender][_transferID] == false);
    require(transferLog[_transferID].approved == false);
    
    approvals[msg.sender][_transferID] = true;
    transferLog[_transferID].approvalCount++;
    
    //when number of required approvals is met, the transfer is completed
    if(transferLog[_transferID].approvalCount >= reqApprovals){
        transferLog[_transferID].to.transfer(transferLog[_transferID].amount);
        transferLog[_transferID].approved = true;
        balance = balance - transferLog[_transferID].amount;
    }
    
}

}

[DavidV]

I’m consistently getting an error when I try to deploy this contract, Even Filips final code does not run.

creation of MultiSigWallet errored: Error encoding arguments: Error: invalid BigNumber string (argument=“value”, value="", code=INVALID_ARGUMENT, version=bignumber/5.3.0)

on Filips code I get :

creation of Wallet errored: Error encoding arguments: Error: expected array value (argument=null, value="", code=INVALID_ARGUMENT, version=abi/5.3.1)

Obviously has something to do with how I input the address when I deploy the contract. Is there now way to create three input lines? This is obviously a silly mistake I’m making but some help will be appreciated!

Been real hard trying to figure this out without the basics running.

[DavidV]

OK So I got it to run by do this :

 constructor(uint _limit, address  _owner1, address  _owner2, address  _owner3) {
     
     
         owners= [_owner1, _owner2, owner3];
          owners.push(_owner2);
           owners.push(_owner3);
           owner1= _owner1;
           owner2= _owner2;
           owner3= _owner3;
           
         
     
     
     limit = _limit;


}

But how would you add an infinite amount of owners to the constructor as an array? if you can’t the point of asking how many approvals are a bit null and void. its obviously at least two.

[jak]

I was able to do some pre-approval multi-sig wallet, which admittedly has little value other than as a disposable wallet for folks like Michael Saylor (hodler’s ).

So I’ll post my code for the pre-approved wallet now and then come back after the react course and do/repost it properly as revision for Solidity 201

pragma solidity 0.7.5;

contract MultiSigWallet {
    
    mapping(address => bool) signature;
    mapping(address => uint) balance;
    
    // Deposit Made to wallet
    event depositMade(address indexed depositedTo, uint amount);
    // Transfer is signed by an owner
    event transferSigned(address indexed signedBy);
    // Transfer is Made
    event transferMade(address indexed sentBy, address indexed sentTo, uint amount);
    
    address ownerAddress1;
    address ownerAddress2;
    address ownerAddress3;
    uint approvalsRequired;
    uint totalSigs = 0;
    
    constructor (address _ownerAddress1, address _ownerAddress2, address _ownerAddress3, uint _approvalsRequired) {
        ownerAddress1 = _ownerAddress1;
        ownerAddress2 = _ownerAddress2;
        ownerAddress3 = _ownerAddress3;
        approvalsRequired = _approvalsRequired;
    }
    
    address[3] ownerWallets = [ownerAddress1, ownerAddress2, ownerAddress3];
    
    function addSig() public returns (bool){
        require((msg.sender == ownerAddress1) || (msg.sender == ownerAddress2) || (msg.sender == ownerAddress3));
        require(signature[msg.sender] == false);
        signature[msg.sender] = true;
        emit transferSigned(msg.sender);
        totalSigs++;
        return signature[msg.sender];
    }
    
    function deposit() public payable returns (uint) {
        balance[msg.sender] += msg.value;
        emit depositMade(msg.sender, msg.value);
        return balance[msg.sender];
    }
    
    function transfer(address payable recipient, uint amount) public {
        require(totalSigs >= approvalsRequired);
        require(balance[msg.sender] >= amount);
        uint previousSenderBal = balance[msg.sender];
        balance[msg.sender] -= amount;
        balance[recipient] += amount;
        assert(balance[msg.sender] == previousSenderBal - amount);
        emit transferMade(msg.sender, recipient, amount);
    }
    
    function getBalance() public view returns (uint) {
        return balance[msg.sender];
    }
}

[Gomez1333]

Greetings everyone! I just got to the project part of the course and I would like to tackle it without any hints from Filip so I would like to start with a draft of what multisigWallet should be and what functions it should have. I will outline my idea and I would like to hear a feedback and critique on my “draft”.

List of features:

1) Deposit funds public function

• function, that allows ANY adress to deposit funds into the wallet. This should be function acessible for anyone acessing the multiSig Wallet smart contract.

1.5) because of 1), I will need walletBalance uint to exist ( balance of the contract)

2) Get Balance public function

• I know it is not required, but I feel it is mandatory to have “get Balance” function in a wallet.

3) Constructor
-to create addresses of the owners and number of approvals needed for transaction to be approved

3.5) function showOwners
-not necessary, but I think it would be nice to have a function that would show which addresses “owns” the multiSigWallet.

4) function createRequest

• this function will create a Request: Request will require these inputs: Receiver Address, amount.
• output wil be struct, which contains following: Receiver Address, amount, Number of approvals(=1) and transaction ID. This will be stored in “Array of structs” which contains all pending transactions that needs to be approved.

4.5) because of 4), we will also need to declare Array of structs called “pendingTransactions”.

5) Function listOfPendingTransactions

• this function will list all pending transactions

6) Function signTransaction

• this function will allow authorized users to sign Transaction with their signature. it will require input of transaction ID. Such function can be sucesfully called only once, second time it will revert with "You already signed that transaction

6.5) If condition
at the end of each sucesfull signature of transaction, there will be IF condition. It will check if enough signatures are gathered, it will execute sendTransaction

7) function sendTransaction
This function will send approved transaction. It should be private and no way directly acessible (not even to owners of the walled)

This is my initial draft. I will be happy for all comments.

[mcgrane5]

hey @Gomez1333. brilliant idea to make a draft like this. i think you should adjust only one thing. you should make the make the deposit function onlyOwners as the owners should be the only people who can deposit into the wallet and createTransfers etc. to me it doesnt make sese for someone who does not own the wallet to deposit into it.

Also u should create a withdraw function restricted to onlyOwners that allows the owners to withdrae their funds from the wallet smart contract. without the withdrawal function an owner will be able to deposit and create transfers but they would never be able to take their funds out of the wallet. other than these two things the outline of your contratc look very good excited to see it

[Gomez1333]

Here is my version of the Multisig wallet. It tried to meet all the requirements from the assignement and added a little bit of my own functions I deemed were needed. Also thanks @mcgrane5 for tips, I included them in my project. I am looking forward to hear comments and critique about my first project.

pragma solidity 0.7.5;
pragma abicoder v2;


contract multiSigWallet  {
    
    //array containing addresses of ownders
    address[] listOfOwners;
    // number of approvals needed for transaction to go through
    uint numberOfApprovals;
    // Total balance of the wallet
    uint walletBalance = 0;
    
    // my way of generating ID. Each time new tx is created, it will increment by 1
    uint IDcounter = 0;
    
    // struct that represents the transaction request. It contains address of receiver, amount, transaction ID, sender address and signature tracker.
    struct pendingTransaction{
        address payable receiver;
        uint amountSent;
        uint txID;
        address sender;
        bool sig;
    }
    //array of structs that contains all pending transactions
    pendingTransaction[] pendingTransactions;
    
    // mapping used to keep track of personal deposits of each owner
    mapping(address => uint) personalBalance; 
    
    //events that tracks when ethere was deposited or sent
    event depositDone(uint amount, address indexed depositedTo);
    event transferMade(address sentFrom, address sentTo, uint amount);
    
    // modifier for the function that checks if sender is the owner
    modifier onlyOwners {
        require (checkOwners(msg.sender), "Only Owners have access to this function");
        _;
    }
    
    //initialization of the owner addresses
    constructor(address owner1, address owner2, address owner3, uint _numberOfApprovals){
        listOfOwners.push(owner1);
        listOfOwners.push(owner2);
        listOfOwners.push(owner3);
        numberOfApprovals = _numberOfApprovals;
    }
    
    // function that checks if sender is owner
    function checkOwners(address user) internal view returns(bool){
        if ((user==listOfOwners[0])||(user==listOfOwners[1])||(user==listOfOwners[2])){return true;}
        else { return false;}
    }
    
    //deposit function
    function deposit() public payable onlyOwners  returns (uint){
       walletBalance += msg.value;
       personalBalance[msg.sender] += msg.value;
       emit depositDone(msg.value,msg.sender);
       return walletBalance;
    }
    
    //function that allows to get balance of the wallet
    function getBalance() view public returns(uint){
        return walletBalance;
    }
    
    //function that creates request for the payment and stores it in arrays of structs called "pendingTransactions".
    function createRequest(address payable receiver, uint amount) public onlyOwners {
        pendingTransactions.push(pendingTransaction(receiver, amount, IDcounter,msg.sender,false));
        IDcounter = IDcounter+1;
    }
    
    //function that will list all pending transactions
    function showPendingTransactions() public view returns(pendingTransaction[] memory){
        return(pendingTransactions);
    }
    
    //function that allows owner to sign pending transaction. It will check if owner did not already signed it and then transfer funds and delete the pending transaction from the list
    function signTransaction(uint ID) public onlyOwners {
        require(msg.sender != pendingTransactions[ID].sender, "You already signed this transaction");
        pendingTransactions[ID].sig = true;
        if (pendingTransactions[ID].sig) {
            transferFunds(pendingTransactions[ID].receiver, pendingTransactions[ID].amountSent);
            emit transferMade(pendingTransactions[ID].sender, pendingTransactions[ID].receiver, pendingTransactions[ID].amountSent);
            delete pendingTransactions[ID];
        }
    }
    
    //function used to transfer funds
    function transferFunds(address payable receiver, uint amount) private {
        require(walletBalance>= amount, "balance not sufficient");
        walletBalance -= amount;
        receiver.transfer(amount);
    }
    
    //withdrawal function for the owners
    function withdrawFunds(uint amount) public onlyOwners payable {
        require(walletBalance>= amount, "Wallet balance not sufficient");
        require(personalBalance[msg.sender]>= amount, "personal balance not sufficient");
        walletBalance -= amount;
        msg.sender.transfer(amount);
    }
}


    
    

[pok]

Hello everyone. This is my version of implementation of the project “Multi-sig Wallet” for this course. Please provide me feedback on this code and ways to improve it.
Thank you.

pragma solidity 0.7.5;
pragma abicoder v2;

contract Bank{
    
    mapping(address => uint) balance;
    mapping(address => bool) signers;
    
    mapping(uint => mapping(address => bool)) isConfirmed;
    
    address[] public owners; 
    uint approvalRequired;
    uint walletBalance;
    
    struct Transaction{
        address payable to;
        uint value;
        uint numberOfConfirmation;
        bool executed;
    }
    
    Transaction[] public transactions;
    
    constructor(address[] memory _owners, uint _noOfConfirmation){
        require(_owners.length > 0);
        require(_noOfConfirmation > 0);
        
        for(uint i=0; i<_owners.length; i++){
            address owner = _owners[i];
            
            require(owner != address(0));
            require(!signers[owner]);
            
            signers[owner] = true;
            owners.push(owner);
        }
        
        approvalRequired = _noOfConfirmation;
    }
    
    event depositDone(uint amount, address depositedTo);
    
    modifier onlyOwner{
        require(signers[msg.sender]);
        _;
    }
    
    modifier notConfirmed(uint _txIndex){
        require(!isConfirmed[_txIndex] [msg.sender]);
        _;
    }
    
    modifier txExists(uint _txIndex) {
        require(_txIndex < transactions.length);
        _;
    }
    
     modifier notExecuted(uint _txIndex) {
        require(!transactions[_txIndex].executed, "tx already executed");
        _;
    }
    
    function deposit() public payable returns(uint){
        balance[msg.sender] += msg.value;
        walletBalance += msg.value;
        emit depositDone(msg.value, msg.sender);
        return walletBalance;
    }
    
    function submitTransaction(address payable _recipient, uint _amount) public onlyOwner{

         transactions.push(Transaction({
            to: _recipient,
            value: _amount,
            numberOfConfirmation:0,
            executed: false
        }));
    } 
    
    function AcceptTransaction(uint index) public onlyOwner notConfirmed(index) txExists(index) notExecuted(index){
        Transaction storage transaction = transactions[index];
        isConfirmed[index] [msg.sender] = true;
        transaction.numberOfConfirmation += 1;
    }
    
    function transfer(uint index) public onlyOwner payable txExists(index){
        Transaction storage transaction = transactions[index];
        require(transaction.numberOfConfirmation >= approvalRequired);
        require(walletBalance >= transaction.value);
        
        transaction.to.transfer(transaction.value);
        
        transaction.executed = true;
        
    }
    
    function getBalance(address _get) public view returns(uint){
        return balance[_get];
    }
    
}   

[Random]

Hi Evan, @mcgrane5

Wow!

Thank you very much for your detailed response

Apologies for my delay in getting back to you, I appreciate the time you took out for me

I will sift through your suggested improvements very carefully

However, as I understood from the project spec, their shouldn’t be an addOwner() function as then you could potentialy add one of your own addresses as an owner and approve transactions?

And what would be the purpose of having a seperate internal balance for each address? Surely that defeats the purpose of a multi-sig wallet? The way I understood it was that any owner can request to transfer the contract balance, and any of them can approve the transfer, and a certain amount of approvals are needed to complete the request. I may have misunderstood aha?

I’m very impressed with the level of detail you have gone into my project. Thank you for the security fixes too, I will certainly add those.

And thank you overall Evan

[mcgrane5]

hey @Random. So the whole point of a multisig wallet is for extra security really. The way i see it is this (i suppose it subjective at the end of the day). So how they can work is “I” am the owner of the wallet (the main owner e.g the contract deployer) and i can choose some signatories (or other people) that i trust to also have owner rights in the wallet so that they can aprrove and transfer requests that i make, the other signatories can also make tarnsfers etc. The reason that hard coding in the owners is bad is because of “flexibility” for lack of a better word. Say i am a signatory but then in a years time i for some reason dont want to be one any more. If i hard code in the owners then this is a flaw i would be stuck in the owners set for ever unless the contract code was changed and was redeployed in my opinion should be avoided at all costs. So there should be an option to remove me as a signatory and then another option to add another signatory if needs be. One thing you should always do is to try your best to “not” hard code things in any type of program in any language. It doesnt make a difference at all in terms of preformance but it makes a huge difference in your codes ability to stand the test of time and to be flexible as thing need to change.

As for the seperate balance mapping the reason for this is to keep track of which funds locked into the wallet smart contract belong to. In remix you have an account and it always starts off with 100 test ether. If i deposit 1 ether into the multisig wallet my remix balance goes to 99 ether but this is not the same thing at all as my balance in the multisig wallet. My balance in the multisig wallet would be 1 ether. Creating a balance maping lets me keep track of this. However if i switch to another account and deposit 2 ether, then the total funds locked into the wallet SC is 3 ether. But 1 eth belongs to address 1 and 2 ether belongs to address 2. The only way to keep track of this distribution is with the seperate balance mapping, and then you can store the total funds locked into the multisig contract as a whole with the global “contractBalance” var.

However it all depends at the way in which you interpret it. Perhaps you want to have your wallet in such a way where the main owner (person who deployed the contract) to be the only person who can actually make transfers and then the other owners are only used as signatories for transfers. In this case then the balance mapping would not be nessecary. But personally for me i wanted to have the entire owners set to have deposit and createTransfer priveleges

The difference between these two balances is very key in all contracts. I hope i explained it ok.

[DavidV]

Hi Everyone, so this is my first try without looking at any of the other code or ideas that Filip gave. As we do not really have an interface I wasn’t sure how the approvals of the transaction should work. In my wallet as as long as two authorised signatures have signed then withdrawals and transfer can take place. if you change the signatures the “NOT AUTHORISED” message. I’m working on the solution that uses the template, I thought I 'd post this as I’d like feedback on my logic in this one. Its probably to the most eloquent solution … but I’m learning!

pragma solidity 0.7.5;
// pragma abicoder v2;

contract MultiSigWallet{

address public creator;
uint public approvals;
bool public sig1;
bool public sig2;
bool public sig3;
uint limit;


  bool[] public sigs;
  address[] internal owners; 
  
  address public owner1;
  address public owner2;
  address public owner3;
  
    
 
    //  owners[0] = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
    //  owners[1] = 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4;
    // owners[2] =  0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db;

 constructor(uint _limit, address  _owner1, address  _owner2, address  _owner3) {
     
     
         owners= [_owner1, _owner2, _owner3];
        //   owners.push(_owner2);
        //   owners.push(_owner3);
           owner1= _owner1;
           owner2= _owner2;
           owner3= _owner3;
           limit = _limit;


}

struct transferStruct{
address payable addressTo;
address addressFrm;
uint amount;
uint id;
bool hasbeensent;

}

transferStruct[] transfersRequests;

function getSigs1(address _sig1) public returns(bool) {

if (_sig1 == owners[0] || _sig1 == owners[1] || _sig1 == owners[2] ) {
       sig1 = true;
     
      }
       
         else sig1 = false;
         return sig3;
  
   }
   
    function getSigs2(address _sig2) public returns(bool) {

   if (_sig2 == owners[0] || _sig2 == owners[1] || _sig2 == owners[2]  ) {
       sig2 = true;
       }
       
        else sig2 = false;
  return sig2;
   }
   
    function getSigs3(address _sig3) public returns(bool) {

   if (_sig3 == owners[0] || _sig3 == owners[1] || _sig3 == owners[2] ){
   sig3 = true;
  }
    
     else sig3 = false;
    return sig3;
   }

event checkcheck(uint checks);

  modifier onlyOwner {
      
    
    uint checks = 0;
       
       if (sig1 == true) checks = checks+1;
       if (sig2 == true) checks = checks+1;
       if (sig3 == true) checks == checks+1;
       
       emit checkcheck(checks);
       
  require(checks>1, "NOT AUTHORISED!");
  string memory Authorised = "Authorised";
  _; //run the function;
  }

mapping(address => uint)balance;

event depositedFunds(uint amount, address depositedTo);
event transferDone(address transferFrom, address transferTo, uint amount);
event withdrawDOne(uint amount);

function deposit() public payable returns (uint) {

   balance[msg.sender] += msg.value;
   emit depositedFunds(msg.value, msg.sender); 
   return balance[msg.sender];

}

function contractBalance() public view returns (uint) {

return address(this).balance;

}

function withdraw( uint amount) public onlyOwner returns (uint) {

 require(balance[msg.sender] >= amount,"Balance not Sufficient!");
 
  uint previousAddressBalance = balance[msg.sender];
  payable (msg.sender).transfer(amount);
  balance[msg.sender]-= amount;
  
  
  assert(balance[msg.sender] == previousAddressBalance - amount);
   return balance[msg.sender];

}

function getBalance() public view returns (uint) {
return balance[msg.sender];

}

function transfer(address recipient, uint amount) public payable onlyOwner {

 require(balance[msg.sender] >= amount);
 require(msg.sender  != recipient);
 
 uint previousSenderBalance = balance[msg.sender];
 
 _transfer(msg.sender,recipient, amount);
 

assert(balance[msg.sender] == previousSenderBalance - amount);
 emit transferDone(msg.sender, recipient, amount);

}

function _transfer(address from, address to, uint amount) private {

 balance[from]-= amount;
 balance[to] += amount;

}

// function transferRequests()
}

[DavidV]

OK, so I have also done this wallet using the method Filips spoke about with the mapping. I see now that the only way to check wether an owner has voted twice is by using the double mapping method. I have hoever not used an array for the owners as they have to be inputted manually and iets easier to add them as individual variable , each with their own line. the rest works similarly to Filips code. My transaction executes from the approvals function once the 2nd approvals has been given. my standard Transfer function has been made private.

pragma solidity 0.7.5;
 pragma abicoder v2;


contract MultiSigWallet{
 
    uint limit;
    address public owner1;
    address public owner2;
    address public owner3;
      
    
 constructor(uint _limit, address  _owner1, address  _owner2, address  _owner3) {
     owner1= _owner1;
     owner2= _owner2;
     owner3= _owner3;
     limit = _limit;
}
struct transferRX{
  
    address payable recipient;
    uint    amount;
    uint    id;
    uint    approvalsGiven;
    bool    hasbeensent;
    

}

transferRX[] transferRequests;  


mapping(address => mapping(uint=> bool)) approved;

      modifier onlyOwner {
           
      require( (msg.sender == owner1) || (msg.sender == owner2) || (msg.sender == owner3), "NOT AUTHORISED!") ;
      string memory Authorised = "Authorised";
      _; //run the function;
      }
      
   mapping(address => uint)balance;
   
   event depositedFunds(uint amount, address depositedTo);
   event transferDone(address transferFrom, address transferTo, uint amount);
   event withdrawDOne(uint amount);
   event approvalsGivenAmout(uint amount, string message);
   event transfersSent(string message);
  
function transfersRqst(address payable _recipient1, uint _amount1)  public  onlyOwner {
     
     require(balance[msg.sender] >= _amount1, "NOT ENOUGH FUNDS");
     require(msg.sender  != _recipient1);
     transferRequests.push(transferRX(_recipient1, _amount1, transferRequests.length, 0, false));
     
     
} 

function getTransferRequests() public view returns (transferRX[] memory){
        return transferRequests;
}
  
  function approveTX( uint _id) public payable onlyOwner { 
      
      require(approved[msg.sender][_id] == false, "You've already voted!");
      require(transferRequests[_id].hasbeensent == false,"Transaction has been sent!");
      
      approved[msg.sender][_id] = true;
      transferRequests[_id].approvalsGiven++;
      emit approvalsGivenAmout(transferRequests[_id].approvalsGiven, "approvals has been given.");
      
      if (transferRequests[_id].approvalsGiven >= limit) {
          
          transfer(transferRequests[_id].recipient, transferRequests[_id].amount);
          transferRequests[_id].hasbeensent = true;
          emit transfersSent("The transaction has been sent!");
      }
    
      
  }

  function transfer(address _recipient, uint _amount)  private onlyOwner {
     
     require(balance[msg.sender] >= _amount);
     require(msg.sender  != _recipient);
     
     uint previousSenderBalance = balance[msg.sender];
     
     _transfer(msg.sender,_recipient, _amount);
     
  
    assert(balance[msg.sender] == previousSenderBalance - _amount);
     emit transferDone(msg.sender, _recipient, _amount);
  }
 
 function _transfer(address from, address to, uint amount) private {
     
     balance[from]-= amount;
     balance[to] += amount;
 }
 
 function getBalance() public view returns (uint) {
     return balance[msg.sender];
 }
 
 function  contractBalance() public view  returns (uint) {
    return address(this).balance;
 }
 
  function deposit() public payable returns (uint)  {
       balance[msg.sender] += msg.value;
       emit depositedFunds(msg.value, msg.sender); 
       return balance[msg.sender];
   }
   
   
   
}