Project - Multisig Wallet

If I remember well, you can’t use .push into structs array, but there could be a little trick.

Instead of using push, you can just directly save the values into the position of the array.

Example:

vIni.contributor[vIni.contributor.length] = msg.sender;

Maybe must be improved a little bit, but the basic idea could be that one.

Let me know if this helps :nerd_face:

Carlos Z

2 Likes

hey @B_S. Yeah the way your approaching it is gonna make it difficult your code does look good though. There is a way to push into a struct array but i cant remeber off the top of my head you can there is a post somewhere on ethereum.stack.exchange that has an example thats where i orginally found it. its something to do with declaring your your array with new address[](1) or somethingalong those lines but i suggets setting a global array empty and in the constructor append msg.sender then creat an add and remove owner function to add and remove as you need to. im busy now but i can have alook at your code tomight

1 Like

this, exactly: i think i was having problems with the memory/storage issue reference v copying, but i eventually figured it out. however, i didn’t test thoroughly beyond deployment because i’m on a stricter schedule these days; i was happy just to get rid of all the compilation errors!
anyway, thank you for the pointers; i’ll keep them in mind, esp front end computations.

1 Like

@thecil too
i ended up using

voterInitiatives storage local_vIni = vIni;
        local_vIni.receiver.push(_to);

i think the problem was the storage/memory thing… i’m not sure exactly, although vIni was a struct of array instances, so now i question whether that actually worked as intended…at least the compiler stopped giving me grief haha (i realize that’s not great practice; it’s not something i’d continue if I were to take programming as a career)

haha! i had it right just above for the for loop, i swear!

as much as i’d love to do this, and i know the following is not good practice for coding, i just have to blot everything down and work through it; apparently my thinking is not very clear upfront when it comes to coding projects, although i do enjoy the challenge of sorting the mess all out. how do you end up starting on such a project? i’ve heard to write pseudocode first… i guess i should’ve written a workflow / info-flow or something… bullet-pointed goals certainly (as filip had them in the assignment page)

ah, no worries, but i appreciate your offer; i didn’t stumble across that stack exchange question, but that is an interesting point.
i finally got it working well enough to deploy (see the response above if you’re interested…)
thanks

hey @B_S ok so i knew i had a code which can push to a struct array attribute somewhere in this form and i did some diffing and found it. Not sure if this is wht you after but it is given below

pragma solidity ^0.8.4;
pragma abicoder v2;

contract Wallet {
    // constants
    uint8 constant public MAX_OWNERS = 10;
    
    // enums
    enum TransferState {
        Deleted, // This state should be only reached when the Transfer item is deleted
        Pending,
        Ready, // Transient state
        Completed,
        Cancelled, // Transient state
        Failed
    }
    
    // state variables
    uint public approvalCount;
    uint public id;
    address[MAX_OWNERS] private owners_array; // Used to cleanup approvals (not worth it in termas of gas)
    mapping(address => bool) private owners; // Used for onlyOwner modifier
    mapping(uint => Transfer) public transfers; // Infinitely growing
   
    
    // structs
    struct Transfer {
        address payable recepient;
        uint amount;
        uint creationTime;
        uint approvalCount;
        address[] approvers;
        TransferState state;
    }
    
    // events
    event ReportTransferState(
        uint indexed id,
        address indexed recepient,
        uint amount,
        TransferState indexed state
    );
    
    // modifiers
    modifier onlyOwner() {
        require(owners[msg.sender] == true, "Owner-only operation");
        _;
    }
    
    modifier transferExists(uint _id) {
        require(_id <= id, "Transaction doen't exist yet");
        _;
    }
    
    // constructor
    constructor(uint _approvalCount, address[] memory _owners) {
        require(_owners.length <= MAX_OWNERS, "Too many owners");
        require(_owners.length >= _approvalCount, "Approval count cannot exceed the number of owners");
        
        for(uint i = 0; i < _owners.length; i++) {
            owners[_owners[i]] = true;
            owners_array[i] = _owners[i];
        }
        
        approvalCount = _approvalCount;
        id = 0;
    }

    //public
    function deposit () public payable  {}
    
    function getTransfers(uint _id) public view returns (Transfer memory) {
        return transfers[_id];
    }
    
    function withdraw (uint amount) public payable onlyOwner {
        payable(msg.sender).transfer(amount);
    }
    
    function getBalance () public view returns (uint) {
        return address(this).balance;
    }
    
    // Should create a new Transfer object
    // Only owner is allowed to create TransferState
    // Do not create transfers if the wallet balance is lower than the specified amount
    function createTransfer (address payable recepient, uint amount) public onlyOwner returns (uint){
        require(address(this).balance >= amount, "Insufficient balance in Wallet");
        
        // Create a new transfer object
        // approvalCount is set to 1 because the creator is assumed to have approved the transfer he has created
        Transfer memory new_transfer = Transfer(
            recepient,
            amount,
            block.timestamp,
            1, 
            new address[](0),
            TransferState.Pending
        );
        
       
        //Register the new transfer
        transfers[id] = new_transfer;
        
        
        processState(id);
        
        id++;
        
        return id - 1;
    }
    
    // Approves existing transfers which are in Pending state
    // Only owner can approve transfers
    // Each address can approve transation only once
    function approveTransfer (uint _id) public onlyOwner transferExists(_id) returns (uint){
        require(transfers[_id].state == TransferState.Pending, "Only pending transfer can be approved");
        // require(approvals[_id][msg.sender] == false, "This address already approved this transaction");
        for (uint i = 0; i < transfers[_id].approvers.length; i++) {
            if(transfers[_id].approvers[i] == msg.sender) revert();
        }
        
        // Change transfer states
        transfers[_id].approvalCount++;
        transfers[_id].approvers.push(msg.sender);
        
        assert(
            transfers[_id].approvalCount <= approvalCount &&
            transfers[_id].approvalCount > 1
        );
        
        processState(_id);
        
        return _id;
    }
    
    // Revokes approval from existing transfers which are in Pending state
    // Only owner can revoke transfers
    // Each address can revoke transation only if it has previously approved it
    function revokeTransfer (uint _id) public onlyOwner transferExists(_id) returns (uint){
        require(transfers[_id].state == TransferState.Pending, "Only pending transfer can be revoked");
        for (uint i = 0; i < transfers[_id].approvers.length; i++) {
            require(transfers[_id].approvers[i] == msg.sender);
        }
        
        // Change transfer states
        transfers[_id].approvalCount--;
       
        
        assert(
            transfers[_id].approvalCount < approvalCount &&
            transfers[_id].approvalCount >= 0
        );
        
        processState(_id);
        
        return _id;
    }
    
    // Execute transfer that is in Ready state
    function executeTransfer (uint _id) public payable onlyOwner transferExists(_id) returns (uint) {
        require(
            transfers[_id].state == TransferState.Ready ||
            transfers[_id].state == TransferState.Failed,
            "Only Ready or Failed states are allowed"
        );
        
        if (transfers[_id].recepient.send(transfers[_id].amount))
            transfers[_id].state = TransferState.Completed;
        else
            transfers[_id].state = TransferState.Failed;
        
        processState(_id);
        
        return _id;
    }
    
    // Cancels transfer that is in Failed state
    function cancelFailedTransfer (uint _id) public onlyOwner transferExists(_id) returns (uint) {
        require(transfers[_id].state == TransferState.Failed, "Only Failed transfer can be cancelled");
        
        transfers[_id].state == TransferState.Cancelled;
        
        processState(_id);
        
        return _id;
    }
    

    //private
    function processState(uint _id) private {
        Transfer storage t = transfers[_id];
        
        if (t.state == TransferState.Pending) {
            emit ReportTransferState(_id, t.recepient, t.amount, t.state);
            
            if (t.approvalCount == approvalCount)
                t.state = TransferState.Ready;
            else if (t.approvalCount == 0)
                t.state = TransferState.Cancelled;
        }
        
        if (t.state == TransferState.Ready) {
            emit ReportTransferState(_id, t.recepient, t.amount, t.state);
            
            executeTransfer(_id);
        }
        
        if (t.state == TransferState.Failed) {
            emit ReportTransferState(_id, t.recepient, t.amount, t.state);
        }
        
        if (t.state == TransferState.Cancelled || t.state == TransferState.Completed) {
            emit ReportTransferState(_id, t.recepient, t.amount, t.state);
            
            // This delete reduces gas fee from 90642 to 49665
            delete transfers[_id];
            // Further deletion of approvals via iteration through owners_array
            // only increases the gas fee to 69251
        }
    }
}

to save you someill explain the pushing to the struct array. so we initialise the struct like this this is the transfet struct

// structs
    struct Transfer {
        address payable recepient;
        uint amount;
        uint creationTime;
        uint approvalCount;
        address[] approvers;
        TransferState state;
    }

in this example i keep track of the approvers using an array initially empty and each time a user approces a transsction i append that address to the array and then make a requirement that the user approving is not in this array for that transaction id. to initalise the transfer struct in the create transfer function i use that new address[](0) thing i mentioned esterday

 function createTransfer (address payable recepient, uint amount) public onlyOwner returns (uint){
        require(address(this).balance >= amount, "Insufficient balance in Wallet");
        
        // Create a new transfer object
        // approvalCount is set to 1 because the creator is assumed to have approved the transfer he has created
        Transfer memory new_transfer = Transfer(
            recepient,
            amount,
            block.timestamp,
            1, 
            new address[](0),
            TransferState.Pending
        );
        
       
        //Register the new transfer
        transfers[id] = new_transfer;
        
        
        processState(id);
        
        id++;
        
        return id - 1;
    }

thuis allows us to initalise an empty array eah time we create a transfer but using tis form by creating a new empty array allows us to push to it anytime we want for any given transfer. consider the approve function where we do this

 function approveTransfer (uint _id) public onlyOwner transferExists(_id) returns (uint){
        require(transfers[_id].state == TransferState.Pending, "Only pending transfer can be approved");
        // require(approvals[_id][msg.sender] == false, "This address already approved this transaction");
        for (uint i = 0; i < transfers[_id].approvers.length; i++) {
            if(transfers[_id].approvers[i] == msg.sender) revert();
        }
        
        // Change transfer states
        transfers[_id].approvalCount++;
        transfers[_id].approvers.push(msg.sender);
        
        assert(
            transfers[_id].approvalCount <= approvalCount &&
            transfers[_id].approvalCount > 1
        );
        
        processState(_id);
        
        return _id;
    }

notice how i first require that
for (uint i = 0; i < transfers[_id].approvers.length; i++) { if(transfers[_id].approvers[i] == msg.sender) revert(); }
this prevents a user apprving twice. then to push the user to the approvals array if he hasnt alread approve we simply call transfer requests of a paticular id and push to the approvers. see the code above. so this is how you can push to arrays in structs. i know your examle is different but you can work around. however in terms of gas this is innefficen there is better ways such as mappings which prevent us from having to do for loop checks using arrays. you should try use loops as little as possible.

the best approach is to creatt a few mappings. an approval mapping that maps an user account to a paricular transaction to a true of flase output. then a isOwner mapping which maps a user to a true of false output. consider below

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

//the double mapping lets us chdck if and address has approve a patricular transaction or not
//approvals[msg.sender][transactionID] = true or false

//the other two mappings are used to determine if a user is a walllet owner and saves us from doing
//for loop checks

we would init a struct like this

 struct Transfer{
        string ticker;
        uint amount;
        address sender;
        address payable receiver;
        uint approvals;
        bool hasBeenSent;
        uint id;
        uint timeOfCreation;
    }

we can be dyncamic in how we add owners so we can make an anowner and removeowner function consider below

function addUsers(address _owners) public onlyOwners
    {
        require(!isOwner[_owners], "casnnot add duplicate owners")
        owners.push(_owners);
        ownerIndex[_owners] = owners.length - 1;
        
        //from the current array calculate the value of minimum consensus
        limit = owners.length - 1;
    }
    
    //remove user require the address we pass in is the address were removing
    function removeUser(address _user) public onlyOwners
    {
       require(isOwner[_user], "user not an owner")
        
        owners[ownerIndex[_user]] = owners[owners.length - 1];
        owners.pop();
        limit= owners.length - 1;
    }

see here we have no loops saves us gas. For the approval mapping we can set up the approval function so that we require approvals is false for the function caller (msg.sender) . this is just a small idea of beter optimisation fo ryou you can play around with these ideas yourself

1 Like

@B_S i just want to say that your solution i svery good it is a lot beter than mine when i originally this this course. however if you would like to see a robust multisig wallet code consider my code below. it is competely dynamic and i implement a factory contract to allow user to create wallet instances each as their own individual smart contract. this is akin to making multiple wallets on metamask. it also supports ERC20 tokens aswell as ethereum and users can add any existing erc20 token they want aslong as they have the contratc address for the token. study the code velow you might pick up a thing or two that will be useful to you in the other courses here

https://github.com/mcgraneder/Etherem-MultiSigWallet-Dapp/blob/MultiSigFactory/contracts/Multisig.sol

(note if your reading this repo make sure your in the fsactory branch not main) just note thst ig your not going to use your contract in an actual dapp dont bother with the factory contract because its really only useful if you have a front end ui to actuall create new wallets if you just want to play around in remix a single contract will suffice more than enough

1 Like

@B_S, @thecil, @mcgrane5

Actually, you can… as long as the array isn’t a fixed-size array. To use push, the array must be a dynamically-sized storage array, and whether it’s nested within some other data structure within storage or not doesn’t seem to matter. You can’t push to a dynamically-sized memory array (i.e. an array created within the local scope of a function).

Here’s a contract to demonstrate that it’s possible…

  • First, add some struct instances to the groups array by calling addGroup() several times, each time inputting an array of different numbers. This gives you an array of Group instances, each containing a numbers property with an array of numbers.

  • Next, add the same new number to each numbers array in each Group instance, by calling addNum(). The for-loop iterates over the groups array, pushing the new number to each array on each iteration.

  • Finally, you can check which numbers each array contains, by calling getGroup() with the index number of the Group instance you want to check.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

contract StructArrayExample {

    struct Group {
        uint[] numbers;
    }

    Group[] groups;

    function addGroup(uint[] memory nums) public {
       Group memory newGroup;
       newGroup.numbers = nums;
       groups.push(newGroup);
    }
    
    function addNum(uint num) public {
        for (uint8 i = 0; i < groups.length; i++) {
            groups[i].numbers.push(num);   
        }
    }
    
    function getGroup(uint index) view public returns(uint[] memory) {
        return groups[index].numbers;
    }

}

This is obviously very simplistic, but I think it demonstrates well what can be done.

3 Likes

hey @jon_m yeah i was thinking you could i actually dug up spme old code wher ei had done this its slightly different that yours. when you initialising the struct you need to use new address[](0) and this will allow you to push to the array in the form of struct[index].array.push(data). this is another new way that i have not seen i n your code thanks for sharing

1 Like

Don’t you mean creating a local memory array, rather than a struct?
I think this code with the new keyword is used in situations where you need to create a local memory array whose fixed size needs to vary in length. The problem with local memory arrays is that they won’t work unless their length is defined before adding values to them… I think that’s right, but as I said to @B_S, this is a complex area, and one I’m still getting to grips with myself :sweat_smile:

2 Likes

hey @ jon_m. ehhh not really i use a maping to map to transfer struct instesd of storing them in a array of transfers. but i did initialise each transfer to memory on creation. this has its drawbacks as you cannot make a getter function to query the array in the struct. you can get it other ways but its more annoying. i dont like this method at all but just showing one possible way of how you can append data to an array in a struct. yours above seems easier thoug an makes use of the array of structs intead of my mappings approach. but anyway what i mean is if you declsre a struct like this.

 struct Transfer {
        address payable recepient;
        uint amount;
        uint creationTime;
        uint approvalCount;
        address[] approvers;
        TransferState state;
    }

    //this is th eonly way we i can do it this way where we make a mapping to a transfer instance
    //instead of storing the transfers in an array. i dont like doing this but again there is much better ways
    // but showing a "possible solution". should avoid arrays in structs 
    mapping(uint => Transfer) public transfers

and initialise it like this

Transfer memory new_transfer = Transfer(
            recepient,
            amount,
            block.timestamp,
            1, 
            new address[](0),
            TransferState.Pending
        );
        
       
        //Register the new transfer
        transfers[id] = new_transfer;

if we initalise everything like this then we can append to the transfer aray attribute.

2 Likes

Hey @mcgrane5,

I understand now :smiley: … and I’ve been looking at the same explanation in your post to @B_S earlier.

This is very interesting. I had only seen new used to create a local memory array e.g.

address[] memory localArray = new address[](x);

…where x references some value which determines the local array’s required length each time the function is executed.

But I can see now that yours is another use, which enables a struct instance to be created with an empty array as one of its properties. The interesting thing is that it sets the array as a dynamically-sized array which, as you say, allows push to be used :ok_hand:

I’ve modified the contract I posted earlier for this, and it now does the following:

  • addGroup() creates a new struct instance with an empty address array, and adds it to the groups array.
  • addAddress() pushes the caller’s address to each array in each of the struct instances currently within the groups array.
// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

contract StructArrayExample {

    struct Group {
        address[] addresses;
    }

    Group[] groups;

    function addGroup() public {
       Group memory newGroup = Group(new address[](0));
       groups.push(newGroup);
    }
    
    function addAddress() public {
        for (uint8 i = 0; i < groups.length; i++) {
            groups[i].addresses.push(msg.sender);   
        }
    }
    
    function getGroup(uint index) view public returns(address[] memory) {
        return groups[index].addresses;
    }

}

Thanks for bringing this different use of the new keyword to my attention :slightly_smiling_face:

2 Likes

hey @jon_m hahahaha yes asolutley brillaint now you can use array of structs and a getter function for the array. i really love the combination of ideas to arrive at the better solutions thats great stuff hahaha thats what coding is all about isnt it. these forums are a whealth of knowledge alright.

2 Likes

But I do agree that storing struct instances in a mapping is usually more appropriate. I’ve stored them in an array, because that’s what @B_S’s original code was doing.

2 Likes

yesh i think so to same here i think there is better ways to get around putting even the array attribute in the struct. but yes because if you wand to do seasrching you dont need a loop although sometimes it can be useful to use the array of struct format especially if you want to return the enitre set of struct instances and some other things too like keeping order but yeah i do like the mapping option myself. Anyways @B_S there is lots for you to go off now i hope you enjoy when you see this. thew multisig is one of the better forums in this regard

2 Likes

Hi @B_S,

This simplifies things because your vIni state variable is now a single voterInitiatives struct instance, instead of an array of struct instances. But you don’t need to create the local storage pointer; you can push the values directly to the arrays within vIni as follows…

function requestXferVote(string memory _rationale, address payable _to, uint _toXfer) public onlyContributors {
    vIni.receiver.push(_to);
    vIni.amt.push(_toXfer);
    vIni.requestID.push(_rationale);
}

Even if you do use the storage pointer, the values are still pushed directly to the arrays in persistent storage, because the pointer does not create a local copy of vIni. So, your last line in this function body is redundant, and can be removed…

Here’s another version of my example contract, demonstrating how values can be pushed to arrays within a single struct instance, instead of multiple struct instances within a single array.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

contract StructArrayExample {

    struct Log {
        address[] addresses;
        uint[] nums;
        string[] names;
    }

    Log myLog;
    
    function updateLog(uint num, string memory name) public {
        myLog.addresses.push(msg.sender);
        myLog.nums.push(num);
        myLog.names.push(name);
    }
    
    function getLog() view public returns(address[] memory, uint[] memory, string[] memory) {
        return (myLog.addresses, myLog.nums, myLog.names);
    }
}
2 Likes

I think how you tackle the planning stage very much depends on how you best organise your thoughts, and how methodical and analytical you are while you’re actually coding.

Whichever approach you take, it should aim to keep things as simple as possible. The final product almost certainly won’t be simple, but by consciously attempting to find the simplest solutions to problems, you are then less likely to find yourself beaten by complexity.

Depending on the size and scope of the project, I like to start by brainstorming and sketching some kind of rough diagram. It’s usually a mix of illustration, flow diagram, pseudocode and notes. I find it helps me to organise my thoughts, and to become aware of some of the advantages, disadvantages and potential difficulties I’m likely to come across with different approaches. Personally, I don’t spend a long time writing a lot of pseudo code, because I like to get stuck into my first draft of real code as soon as possible. But I am aware that what I code at first will most probably undergo lots of rewrites. This also works for me, because I’m good at realising when I need to stop, backtrack and try a different approach with something. If this is something you would find frustrating and hard to do, then a more structured and detailed planning stage would be more advisable.

2 Likes

I think also @B_S you should complete this course then do solidity 201 once you finish thatyou will have enough under your belt to make something cool. I suggest then making a comlete dex app, in 201 for the final project you make a dex algorithm. learn react or use vanilla js to make a front end for this. Same goes here use react or vanilla to maka a frontedn for the wallet.

Go onto github look at other peoples projects get inspiration read good quality profuction code. if your into defi i suggest forking the likes of aave uniswap, 1inch code and playing around with it on your machine to understand their contracts. Trust me youll get a lot of inspiration this way.

Also do dapp programming where you build crypto kitties. then try build your own nft marketpace. the possibilities are endless.

If your into backend learn rust. go over to polkadot and read the substrate docs (substrate is polakdots blockchain language its written in rust) try get a simple UTXO blockhain running from their guide. theres also a full length tutorial on youtube on the pairity channel. parity are the company who built out polakdot they do great stuff.

Dont worry after i finished this course i wrote a big post on the forum asking what could i build i didt know but just give it time and youll come upt with your own ideas in an instance.

2 Likes

Maybe I haven’t quite understood your issue here with the getter, but when I use a mapping, instead of an array, to store my simple struct instances in my simple example, I can retrieve any individual value from my struct arrays using a getter as follows…

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.10;

contract StructArrayExample {

    struct Group {
        uint[] numbers;
    }

    mapping(address => Group) groups;

    function addGroup() public {
        // assigns new struct instance to mapping directly
        // instead of via a local variable saved temporarily in memory
        groups[msg.sender] = Group(new uint[](0));
        // Group memory newGroup = Group(new uint[](0));
        // groups[msg.sender] = newGroup;
    }
    
    function addNum(uint num) public {
        groups[msg.sender].numbers.push(num);   
    }
    
    function getNum(uint index) view public returns(uint) {
        return groups[msg.sender].numbers[index];
    }

}

But maybe by querying the array, you mean something else…? :thinking:

2 Likes

i ltreally did a quick test for it earlier and every time i clocked my getter function it returned nothing at all. it had to change some of the code in that contract so i might have missed something. try it for an array of addresses and see does that make a difference. i may have missed something but it didnt return anything in the quick example i did earlier

1 Like