Changes m_owners and m_ownerIndex to use address instead of uint.

[MicahZoltu]

It is unclear why this code was using uint previously instead of address. If address is what is intended, address should be used. Using address not only makes the intent more clear, it allows the static analyzer to validate the code.

Also, tooling will now work better with this contract since the types are correct.

If these variables were intentionally uint, I am curious to know the reason why and at the least there should be a comment in the code indicating why they are uint.

[MicahZoltu]

Moved from ethereum/meteor-dapp-wallet#148 per request of @alexvandesande.

[bencxr]

ACK on this, any historical reason not to?

[chriseth]

It was used for optimization purposes.

[MicahZoltu]

Is the optimization still necessary? If working with an address is slower than working with an int then that sounds like a bug that should be addressed in the VM or compiler, not by the dapp.

[chriseth]

Yes, there are several missing features in the optimizer.

[bencxr]

Hey Chris, can you shed some light on the impact of making this change and what is unoptimized?

I am creating a seperate diff to add a public interface to the owners, pending approvals etc, but the interfaces should return an address.

I tested with storage as addresses and deployed the new contract - it was a little larger in size but not significant (and we can use stubs).

[chriseth]

@bencxr the optimizations are probably not worth it. It reduces the code size and execution-time gas costs, but they are probably largely dominated by storage costs anyway.

[MicahZoltu]

Other than a rebase, is there anything I can do to move this PR forward? If it is undesired, let me know so I can close it out. If more discussion is required, is there anyone else that could be @mentioned to get appropriate visibility?

[alexvandesande]

I think the main problem is that this file is old, complex, yet very important. Since, I believe solidity good practices have made sol files much more readable and understandable since then. So no one is volunteering to take the risk of reviewing and updating this code, because a lot of value can be lost by an accidental bug.

Consensys is working on a new wallet app that has a compatible interface but has a simpler more manageable code and it's the Mist teams intention to migrate to that over time..

[MicahZoltu]

Okay, I'll close out this PR then, thanks!