msg.sender provides wrong AccountId on substrate

[h4nsu]

(seems to me like this could be related to #666)

I'm using solang 0.1.9 installed via cargo from crates.io and a custom substrate node (https://github.com/Cardinal-Cryptography/aleph-node/tree/smartnet) which is based on substrate 4.0.0-dev. It looks like msg.sender is providing an incorrect value. This is my minimal example:

contract mytoken {
    function test(address account, bool sender) public view returns (address) {
        if (sender) {
            return msg.sender;
        }
        return account;
    }
}

When I call that function from some address and provide the same address as the account parameter, I'm getting two different values when switching the bool parameter:

[LucasSte]

Hi @h4nsu , Please check this PR. I added your example contract to our mock VM test for Substrate and to our integration tests with Substrate. Everything seems to be working as expected.

I also tested your contract on Substrate's interface and see that the returned addresses aren't equal. Perhaps, this is a issue with Substrate's UI interface. Did you experience msg.sender returning the incorrect address in any other circumstances?

[h4nsu]

Hi @LucasSte, Thanks for the reply. I looked into this PR and I'm not sure it actually tests the behavior I observed. I think this should be something like:

    runtime.function("test", TokenTest(&runtime.vm.caller[..], true).encode());
    assert_eq!(&runtime.vm.caller[..], &runtime.vm.output[..]);

    runtime.function("test", TokenTest(&runtime.vm.caller[..], false).encode());
    assert_eq!(&runtime.vm.caller[..], &runtime.vm.output[..]);

[LucasSte]

Hello @h4nsu, I added the tests you suggested here. Our CI tests passes as well. Do you have any suggestion of integration tests we should be testing to recreate your issue? Integration tests deploy a Solang compiled contract into Substrate and allow us to call its methods. Have a look at the test I added for your issue on my last PR.

[seanyoung]

@extraymond this issue here does not use the instantiate contract api call.

[extraymond]

@seanyoung Ah! I mistakenly remember this issue being the one related to issue666. I'll move my comment there!

[xermicus]

This has been fixed a while ago.