Skip to content
This repository has been archived by the owner on Jun 13, 2023. It is now read-only.

Changes m_owners and m_ownerIndex to use address instead of uint. #148

Closed
wants to merge 1 commit into from
Closed

Conversation

MicahZoltu
Copy link

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.

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`.
@alexvandesande
Copy link

alexvandesande commented May 20, 2016

The correct repo to do this is: https://github.com/ethereum/dapp-bin/

This is not just a categorization issue, it's because we'd rather keep such an important part of the code being reviewed by more people as they are more involved in the solidity security aspect of the wallet contract code. I do not feel comfortable reviewing the security implications of any changes to the wallet.sol contract

I would call attention to this discussion, where you can see that @bencxr and @CJentzsch are also doing some substantial rewrites of the wallet code.

ethereum/dapp-bin#58 (comment)

Thanks for your contributions anyway!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants