Inheritance & External Contracts - Discussion

Ethereum Smart Contract Programming 101
#337

Hi @jeanphi,

When users call the deposit() function, ether is transferred from their external wallet addresses to the contract address. The contract address balance is therefore the total amount of ether belonging to all account holders. The balances in the mapping perform an internal accounting role, and keep track of each user’s share of the total pooled funds. Without them there would be no way of knowing what each individual user’s entitlement was. The ether itself is held by the contract address, and not in the mapping.

When a user calls the withdraw() function, ether is transferred from the contract address to the user’s external wallet address. First of all we perform the internal accounting adjustment to record the reduction in the user’s share of the total pooled funds:

balance[msg.sender] -= amount;

Then we perform the actual transfer of ether from the contract address balance to the user’s external wallet address, using:

<address payable>.transfer(uint256 amount);

When a user calls the transfer() function, no ether is entering or leaving the contract. The total contract address balance doesn’t change. It is purely an internal transfer between users which results in a decrease in the sender’s share of the total contract balance, and an increase, by an equal amount, in the recipient’s share of the total contract balance. It is therefore only necessary to adjust each user’s individual balance in the mapping (two equal and opposite internal accounting adjustments).

You can see a user’s external wallet address balance increase and decrease as ether is either withdrawn from, or transferred to the Bank contract. We can check the total amount of ether held in the contract (the contract address balance) by creating a getter and returning address(this).balance  e.g.

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

Let me know if anything is still unclear, or if you have any further questions :slight_smile:

#338

Thanks @jon_m for replying so quickly!
I think I understand better these concepts now but can you confirm 2 more things for me:

  1. Why Filip’s solution here https://github.com/filipmartinsson/solidity-0.7.5/blob/main/inheritance-assignment/Bank.sol shows a withdraw function that is not actually withdrawing any amount from the user account internal balances mapping?
  2. Why is deposit payable but not withdray? Both need a ETH value as an input that is impacting the contract total balance?
    Thank you!
    JP
1 Like
#339

Hi @jeanphi,

This is an error, the withdraw function in the solution should include the deduction of the withdrawal amount from the user’s balance in the mapping, as follows:

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

In addition, access to the withdraw function shouldn’t be restricted by the onlyOwner modifier. If you think about it, this doesn’t make any sense, because otherwise all users, except for the contract owner, will not be able to withdraw any of the ether they have deposited!

The solution code for the Transfer Assignment is actually correct for both of these points:
https://github.com/filipmartinsson/solidity-0.7.5/blob/main/transfer-assignment-solution.sol

A function only needs to be marked payable when it receives ether from an external address to add to the contract address balance. It might help to think of payable as the code that enables msg.value to be added to the contract address balance, without us having to add any further code for this. In the deposit function, this happens automatically because we have marked the function as payable. The line   balance[msg.sender] += msg.value;   then adds the same value to the individual user’s balance in the mapping, in order to keep a track of their share of the total funds held in the contract.

In contrast, the withdraw function is deducting ether from the contract balance and sending it to an external address. It does this using the address member transfer (which is like a method called on a payable address)…

<address payable>.transfer(uint256 amount);

We should not mark the withdraw function as payable, because it does not need to receive ether from an external address in order to update the contract balance. If we made it payable, then, if the caller sent an ether value to the withdraw function by mistake, as well as calling it with a withdrawal amount, then this ether would be added to the contract balance and potentially lost to the user, because there is no accounting adjustment made to the mapping in the withdraw function for such a deposit (only for withdrawals). However, the function is much securer if we leave it as non-payable, because then, if the caller made the same mistake, the function would revert, and the ether would not be sent.

Just let me know if you have any further questions :slight_smile:

2 Likes
Transfer Assignment
Transfer Assignment
Transfer Assignment
#340

Perfect! This all make sense! Thank you for the precision in your reply!

1 Like
#341

1. What is the base contract?
The base contract is the parent contract

2. Which functions are available for derived contracts?
Public
Internal scoped
State variables

3. What is hierarchical inheritance?
Is when a single contract acts as a base contract for multiple derived contracts

1 Like
#342

Nice answers @CryptoDiddy :ok_hand:

Just one thing…

As with functions, state variables also need to have public or internal visibility to be inherited by derived contracts. State variables marked private are not available. On the other hand, all modifiers and all events in the parent contract are inheried.

Just let me know if you have any questions :slight_smile:

1 Like
#343

I hope I am in the right place and that I have the right idea on Ownable/Destroyable inheritance. Here is what I came up with:

================= Ownable.sol =================
pragma solidity 0.8.4;

contract Ownable {
address public owner;

modifier onlyOwner {
    require(msg.sender == owner);
    _; // run the function
}

constructor() {
    owner = msg.sender;
}

}
================ Destroyable.sol ======================
pragma solidity 0.8.4;

import “./Ownable.sol”;

contract Destroyable is Ownable {

event contractClosed(uint closeBalance, address indexed closeAddress);

function destroyContract() onlyOwner public { 
    uint contractBalance = address (this).balance;
    selfdestruct(payable (owner)); 
    assert(address (this).balance == 0);
    emit contractClosed(contractBalance, owner);
}

}================ helloworld.sol ===============
pragma solidity 0.8.4;

import “./Ownable.sol”;
import “./Destroyable.sol”;

contract Bank is Ownable, Destroyable {
…
}

I can now call destroyContract() from Remix, but only from the original
deployer of the contract, not from any other address, so that appears correct,
and I DO, in fact get my balance back, plus what was in the other address.

I am wondering, though, where do I see the logs again?
I am hoping to see the contract balance that gets sent to the
contract owner when it is destroyed via the event called in Destroyable.sol.

Thanks!

1 Like
#344

Hi, FrankB,
I believe that modifiers are similar to compiler directives in that the modifier code is actually inserted into your function at compile time, therefor it is not really “referenced” publicly or privately.

Then again, I may be wrong…

#345

Well, my Bank contract was working well until I added the Government Interface and broke it!

First question: Why would the addTransaction function be payable? Does it need to be or was that an example?

Second question: Why am I getting this error when I call addTransaction()?
revert
The transaction has been reverted to the initial state.
Note: The called function should be payable if you send value and the value you send should be less than your current balance.
Debug the transaction to get more information.

In Bank contract:

interface IGovernment{
function addTransaction(address _from, address _to, uint _amount) external payable;
}

in transfer() function:
governmentInstance.addTransaction{value: 1 wei}(msg.sender, recipient, amount);

In Government contract:
function addTransaction(address _from, address _to, uint _amount) external payable {
transactionLog.push( Transaction(_from, _to, _amount, transactionLog.length) );
}

Thanks.

Oops. Forgot this line in Bank Contract:

IGovernment governmentInstance = IGovernment(0xdD870f…7392148);

2 Likes
#346

I have the same error after i made the function in government payable and added value to the instance.

#347

Somehow I got mine to work. Did you?
I just kept testing the Government contract stand-alone until it worked, then tried it again from the Bank contract. Not sure why it wasn’t working in the first place!

1 Like
#348

Hi @Samm,

Firstly, apologies for the delay in giving you some feedback on this, and in answering your specific question about the event logs.

Yes… your solution achieves this objective nicely :ok_hand:

Yes… your solution also handles this appropriately. Well done for realising that you needed to make the owner address payable , so that selfdestruct() is called with a payable address and, therefore, any remaining ether in the contract can be transferred to the owner when selfdestruct() is triggered :ok_hand: The remaining contract balance is transferred, and so it’s also correct that the contract owner will receive the sum of all of the individual users’ remaining account balances (as well as their own).

Your coding of the inheritance structure is sound. Note, that we can also streamline the inheritance structure by having Bank explicitly inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable. This will give you a multi-level inheritance structure.

Below are some excerpts from discussions I’ve had with a couple of other students about the issues related to emitting an event for selfdestruct() …

So in your destroyContract function, both the emit statement and the assert statement are unreachable and will not be executed.

Let me know if anything is still unclear, or if you have any further questions :slight_smile:

#349

Thanks so much for the explanation. Especially “…inherit Destroyable only, because it will inherit Ownable implicitly via Destroyable.” and “… the event won’t be emitted because the contract has been destroyed once selfdestruct() has executed, and so the emit statement is never reached…”

These were news to me and very informative. I’m anxious to learn more about proxy contracts!

1 Like
#350

Glad it was informative :slight_smile:
I think you’ll definitely enjoy the Smart Contract Security course…

#351

Hey @Samm,

Once again, apologies for the delay in giving you some answers on this.

It doesn’t have to be. Our example is based on a contractual relationship between a government and a bank. The idea is that the government needs to collect some kind of tax on each banking transaction that is a transfer. To keep things simple in our example, the government is collecting a fixed amount (1 wei) on each transfer. This amount will be automatically transferred from the Bank contract balance to the Government contract balance on each successfully executed transfer transaction. For record keeping purposes, the relevant data for each transfer is also sent from Bank to Government and stored in the government’s transaction log (an array).

Instead of being a tax, another way to interpret this payment could be as payment for some kind of record keeping service i.e. the Bank contract pays the Government contract for storing all of its transaction data.

However we interpret this payment from one contract to the other, it demonstrates (albeit in an overly simplistic way) how we can program automatic interactions between individual contracts for the purposes of both data and value transfer. Hopefully, this example shows the potential for these sorts of programmed interactions to be applied to many different use cases.

But returning to your original question… we could equally exclude the transfer of value from the addTransaction function, if we want it to provide a purely record keeping role at no extra charge (except of course the cost of gas consumption, which in any case is paid to the miners and not to the external contract).

If no value is transferred, then the addTransaction function should not be marked payable, in both Government and the Government interface in Bank.

You should be deploying your Government contract before deploying Bank. If you redeploy Government at any stage, then it will have a different contract address, and so you also need to change the Government address assigned to your governmentInstance in this line of your code in Bank:

If you forgot to replace the previous address of your external contract for the new one, then this will have caused the transaction to revert when you called the transfer function, and will have given you the error message you got. The problem is that the error message you got was just a default one, and so that’s why it wasn’t helpful in pinpointing the error. Your error most probably wasn’t anything to do with the function not being marked payable, or the value exceeding the current balance.

This is easily done, and catches most of us out the first time we make that mistake :sweat_smile:

Adding an error message to your require() statements, is always a good idea, because if the revert is caused by one of them, the message you added will appear as part of the error message you get in the Remix console. If one of your own error messages doesn’t appear, you know that the transaction reverted because of something else.

Let me know if you think that was the cause of the problem or not. But I’m glad to see that you got it to work in the end :slight_smile:

#352

Hey @Joey,

I’ve replied to @Samm regarding this issue. Have a look to see if this solves the problem:

Hopefully that will solve the problem. If not, then post a copy of your code for both contracts and I’ll take a look and investigate further :slight_smile:

#353

Hey guys, is the assignments mentioned in the value calls video the Multi Sig project? or am i missing a video ? :smiley:

1 Like
#354

I wonder if this was caused by re-deploying the Government contract over and over to the same address using “At Address” in Remix? I may have redeployed it using a different address in my list when it finally started working. Can you verify if this could be the case?

Thanks!

#355

1-The base contract is the parent contract. The base contract is referenced by the child like so:
(defining the child contract) contract CHILD is PARENT {…}
2-All public and internal scoped functions are available to derived contracts
3-Heirarchical inheritance is when a single contract acts as a base or parent, for multiple derived or child contracts.

1 Like
#356

contract Ownable {

address public owner;

modifier onlyOwner {
    require(msg.sender == owner);
    _; //run the function
}

constructor() {
    owner = msg.sender;
}

}

pragma solidity 0.7.5;

import “./Ownable.sol”;

contract Destroyable is Ownable {

function destroy() public onlyOwner {
    
    //generate payable owner address
    address payable _owner = payable(owner);
    
    //get total eth in contract
    uint _total = address(this).balance;
    
    //send all ether to owner
    (bool success,) = _owner.call{value:_total}("");
    require(success,"Failed to send Ether");
    
    //self-desctruct contract
    
    selfdestruct(_owner);
}

}

Here is header of Bank contract…

contract Bank is Destroyable {

2 Likes