Try unifying Proxy.mint() and Proxy.mintWithEth() into just Proxy.mint(). (Etc)

[jacob-eliosoff]

Just an idea I was toying with to avoid a bit of redundancy in the Proxy API. I guess could conceivably lead to confusion? But I dunno, seems worth it to me? I won't push by default but curious what you guys think.

[jacob-eliosoff]

(Tests pass btw)

[alexroan]

I like this!

[alcueca]

I think this is cool, I like it.

Do we want to add defaults (maybe ETH as default)?

function mint(minUsmOut) external payable returns (uint) {
    return mint(msg.value, minUsmOut, EthType.ETH);
}

My only concern is that web3 might not work too well with overloading, I've had trouble with that before.

[jacob-eliosoff]

Quick take (may change): I'm not sure about defaults exactly, but I think mint()/fund() of the form you describe (omitting ethIn, and therefore implying ethType=ETH) make sense. But I think burn()/defund() should probably require the ethType argument, since they can't infer it from the other arguments the way mint()/fund() can from whether ethIn is specified.

[jacob-eliosoff]

I tried implementing the mint()/fund() (it looks so easy!) but hit problems, I think to do with msg.value getting reset when one payable fn calls another... I'll give it another try later

[jacob-eliosoff]

I spent a quite ridiculous amount of time debugging the mint()/fund() Proxy fns @albertocuestacanada suggested (since they made sense to me too) and it looks like there are two obstacles:

1. Overloading doesn't work. I can get the single-argument fn to work if I name it mintWithEth(ethIn), but if I name it mint(ethIn) I get runtime (interestingly, not compile-time) name-conflict errors like Error: Invalid number of parameters for "mint". Got 2 expected 3!
2. Even if I give mintWithEth() its own name, I still can't make it just a 1-line call to mint() (as in Alberto's nice example above, and also my intuition - see commented first line in mintWithEth() below), because doing so resets the value of msg.sender to the Proxy contract! I found this quite surprising... Maybe there's some trick like delegatecall to avoid this?

So the end result is these functions are unpleasantly redundant, which was the whole thing this PR was meant to avoid:

    function mint(uint ethIn, uint minUsmOut, EthType inputType) external payable returns (uint) {
        address payer = (inputType == EthType.ETH ? address(this) : msg.sender);
        if (inputType == EthType.ETH) {
            require(msg.value == ethIn, "ETH input misspecified");
            weth.deposit{ value: ethIn }();
        }
        uint usmOut = usm.mint(payer, msg.sender, ethIn);
        require(usmOut >= minUsmOut, "Limit not reached");
        return usmOut;
    }

    function mintWithEth(uint minUsmOut) external payable returns (uint) {
        //return this.mint(msg.value, minUsmOut, EthType.ETH);  // Doesn't work alas...  Note compiler requires "this."
        weth.deposit{ value: msg.value }();
        uint usmOut = usm.mint(address(this), msg.sender, msg.value);
        require(usmOut >= minUsmOut, "Limit not reached");
        return usmOut;
    }

I guess our options are:
a) Find a tweak (delegatecall or something) that avoids the above redundancy
b) Live with the above
c) Scrap the single-argument mint()/burn(); stick with the four functions I originally submitted (still the current branch diffs)
d) Stick with the eight fns currently in master - mint(), mintWithEth(), burn(), burnForEth(), ...

I guess I lean towards c) unless we can get the overloading to work cleanly, but I'm open to your thoughts.

[alcueca]

Oh lord, that's not what I suggested, I'm sorry you went down this rabbit hole.

My question was about a default type for the mint call. I.E, if you don't specify ETHType it assumes ETH. That would be achieved through function overloading. Tbh, I don't even know if I like the idea.

[jacob-eliosoff]

Oh sure don't worry, I knew it wasn't that important, I was just digging to try to understand this stuff.

But how would you implement mint(ethIn, minUsmOut), if not by making it call mint(ethIn, minUsmOut, EthType.ETH)? This is what I found out is not easy to do - the latter ends up getting the wrong msg.sender.

[jacob-eliosoff]

That is:

• User contract U calls on Proxy contract P: P.mint(ethIn, minUsmOut)
• P.mint(ethIn, minUsmOut) simply calls P.mint(ethIn, minUsmOut, EthType.ETH)
• P.mint(ethIn, minUsmOut, EthType.ETH) assumes the USM is going to msg.sender - but msg.sender now returns not U, but P!

[jacob-eliosoff]

I suspect I could make both mint(ethIn, minUsmOut) and mint(ethIn, minUsmOut, inputType) call an internal, non-payable fn _mint(ethIn, minUsmOut, inputType), and that would prevent msg.sender getting mucked up.

[alcueca]

Ah, yeah, my proposal messes up with msg.sender. You would need an internal function that takes a from parameter and passes it on to usm.mint. Definitely not worth it.

[jacob-eliosoff]

Just FYI, making them both call an internal _mint() fn did avoid the msg.sender problems - seems like the latter is only a problem when one payable fn calls another payable fn. _mint() did not however solve the overloading problems: I still get runtime failures from the test if I define/call two mint() fns with different arguments.

So this works:

    function mint(uint ethIn, uint minUsmOut, EthType inputType) external payable returns (uint) {
        return _mint(ethIn, minUsmOut, inputType);
    }

    function mintWithEth(uint minUsmOut) external payable returns (uint) { // But this can't be named mint() too
        return _mint(msg.value, minUsmOut, EthType.ETH);
    }

    function _mint(uint ethIn, uint minUsmOut, EthType inputType) internal returns (uint) {
        address payer = (inputType == EthType.ETH ? address(this) : msg.sender);
        if (inputType == EthType.ETH) {
            require(msg.value == ethIn, "ETH input misspecified");
            weth.deposit{ value: ethIn }();
        }
        uint usmOut = usm.mint(payer, msg.sender, ethIn);
        require(usmOut >= minUsmOut, "Limit not reached");
        return usmOut;
    }

[alcueca]

Yeah, I'm not sure but I think that web3 has an issue with function overloading. Also we ended with as much code as we started, only more complex :P

[jacob-eliosoff]

Yeah basically