Refund function in solidity smart contract not working

Question

I'm trying to implement a HelloWorld smart contract to change 1:1 Ether into a Token (stored in balances) and vice versa - I can call the pay function and everything works out nicely but the refund function doesn't work. Although ethers and tokens (stored in balances) are deducted from the contract, ethers are not sent back to the caller when refund is called - so they disappear. I'm working on the Reopten test net:

So as you can see the contract currently has 0.0088 ether because I've called refund twice with 0.0011, but as you can see the contract doesn't send out ether - so they disapear

https://ropsten.etherscan.io/address/0xaa394f1d64d6193b3189826a7b58e40a56703227

You can also see that there have been two calls to refund from an address where no ethers have been transferred to...

https://ropsten.etherscan.io/address/0x90a700898b596115c03bb9b99b639ad811d792ff

What am I doing wrong?

pragma solidity ^0.4.23;

contract EtherSwap {
mapping(address => uint) balances;

/* Define variable owner of the type address */
address owner;

/* This function is executed at initialization and sets the owner of the contract */
constructor() public { 
    owner = msg.sender; 
}

function() public payable {
    balances[msg.sender] += msg.value;
}   

function query() public constant returns (uint balance) {
    return balances[msg.sender];
}

function query(address id) public constant returns (uint balance) {
    return balances[id];
}

function refund(uint amountRequested) public {
    require(amountRequested > 0 && amountRequested <= balances[msg.sender]);

    balances[msg.sender] -= amountRequested;

    msg.sender.transfer(amountRequested);
}

/* Function to recover the funds on the contract */
function kill() public { if (msg.sender == owner) selfdestruct(owner); }

}

RAW refund transaction:

{"nonce":"0x19","gasPrice":"0x098bca5a00","gasLimit":"0x923a","to":"0xAA394f1d64D6193B3189826A7B58e40a56703227","value":"0x00","data":"0x278ecde10000000000000000000000000000000000000000000000000003e871b540c000","chainId":3}

Signed refund transaction:

{"from":"0x90a700898b596115c03bb9b99b639ad811d792ff","nonce":"0x19","gasPrice":"0x098bca5a00","gasLimit":"0x923a","to":"0xAA394f1d64D6193B3189826A7B58e40a56703227","value":"0x00","data":"0x278ecde10000000000000000000000000000000000000000000000000003e871b540c000","chainId":3}

ABI:

[{"constant":false,"inputs":[{"name":"amountRequested","type":"uint256"}],"name":"refund","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"query","outputs":[{"name":"balance","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"constant":false,"inputs":[],"name":"kill","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[{"name":"id","type":"address"}],"name":"query","outputs":[{"name":"balance","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"},{"inputs":[],"payable":false,"stateMutability":"nonpayable","type":"constructor"},{"payable":true,"stateMutability":"payable","type":"fallback"}]

Answer 1

As an aside, you can simplify that and add a little standards conformance that can be handy in debugging situations.

Remove this

function query() public constant returns (uint balance) {
    return balances[msg.sender];
}

function query(address id) public constant returns (uint balance) {
    return balances[id];
}

and add public to mapping(address => uint) public balances; You'll get a "free" getter function called balances(address). You don't get the fancy overload, but you don't really need it for a client to ask about itself.

You can add event logs

event LogDeposit(address sender, uint amount);
event LogRefund(address receiver, uint amount);

(after the mapping would be "normal").

Then you could update your fallback a little (there just be just enough gas).

function() public payable {
    balances[msg.sender] += msg.value;
    LogDeposit(msg.sender, msg.value);
} 

And the other state-changing function:

function refund(uint amountRequested) public returns(bool success) {
    require(amountRequested > 0);
    require(amountRequested <= balances[msg.sender]);
    balances[msg.sender] -= amountRequested;
    LogRefund(msg.sender, amountRequested);
    msg.sender.transfer(amountRequested);
    return true;
}

I took the liberty of breaking the require up into two blocks and returning a bool. Just a few ideas that can help you and others troubleshoot what's going on.

Hope it helps.

Answer 2

Just found out that you can see the transaction as "internal transactions" on etherscan - this means everything is working as it should

More on "internal transactions" can be found here