txProof function returns "bytes" object instead of "address"

[relyt29]

https://github.com/lorenzb/proveth/blob/d1139277e84c231bf77581a107d0105699747278/onchain/ProvethVerifier.sol#L164

@lorenzb is there an explicit reason why we return a "bytes" object instead of an "address" object here? I can think of no good reason to do so, as it's semantically wrong. for a new contract address, we can just return address 0x0000000000000000000000000000000000000000.

This is tripping up some other code that I'm working on in libsubmarine

[relyt29]

0x0000000000000000000000000000000000000000 is also not actually correct since a new contract creation isn't actually a send to 0x0000...

Maybe I'm wrong but to me it seems broken to return a bytes object instead of an address object, not sure what the "correct" thing to do is here, but returning a bytes object to me seems like lying about the object's type, and makes client code more complicated since clients may now have to convert to bytes themselves for their own use

[lorenzb]

Ideally, we would have to be of type Option<Address> (à la Rust) / Maybe Address (à la Haskell). Unfortunately, Solidity doesn't support this, so instead to is either a bytes with length 20 or a bytes with length 0.

Returning 0x0000000000000000000000000000000000000000 is problematic, because it will prevent the caller from distinguishing between a contract creation tx and a burn tx (e.g. sending money to 0x0).

[relyt29]

I don't like this because then we're lying semantically about what a "to" is in the code, and more importantly, clients have to then implement the logic to convert bytes to address for their own use.

I think a reasonable compromise is to provide a public helper function to clients that does the bytes->address conversion for them: the bytesToAddress function in PR #19 is a good start, though we'd have to figure out what that function does when you pass it an incorrect bytes object (ideally we'd do try catch, though that isn't implemented yet), though this only pushes the problem of what to return on invalid further up the stack from the return of the txProof function to the bytes to address function.

We can also do something a la this solidity question: pick some random address that isn't the burn address like so address constant EMPTY_ADDR = "0xdeadbeef1337cafebabe1337cacaface1337c0de";and then return that magic number...

Or we can return multiple values from the bytesToAddress function, one a boolean success the other the translated address. That's probably the best way to go

But clients need this functionality because solidity clients will be expecting an address, otherwise we just force all of our clients to implement this themselves which a) annoys developers b) means 100 different implementations of the same code c) creates potential for more bugs

[lorenzb]

bool creates_contract and address to (which should be 0x0 whenever creates_contract == true) sounds reasonable to me if we can make it fit into the Solidity stack depth limit.