Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add isInitialized to all state-changing public functions #307

Merged
merged 2 commits into from
May 15, 2018
Merged

Add isInitialized to all state-changing public functions #307

merged 2 commits into from
May 15, 2018

Conversation

bingen
Copy link
Contributor

@bingen bingen commented May 14, 2018

Closes #303.

@bingen bingen requested review from izqui and sohkai May 14, 2018 14:19
@coveralls
Copy link

coveralls commented May 14, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 91.463% when pulling e906b43 on issue303 into e69ceab on master.

sohkai
sohkai approved these changes May 14, 2018
View reviewed changes
Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debating whether or not it's worth it to have tests for this, but I think a single test in each app that makes sure all of the publicly accessible non-view functions revert when the app is uninitialized would be nice.

apps/token-manager/contracts/TokenManager.sol Outdated
@@ -351,7 +351,7 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove
* @param _owner The address that sent the ether to create tokens
* @return True if the ether is accepted, false for it to throw
*/
function proxyPayment(address _owner) payable public returns (bool) {
function proxyPayment(address _owner) payable isInitialized public returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to have isInitialized on this, since it's not a view simply out of conformance to the minime interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I was in doubt about these ones, but I thought it was more aligned with the purpose of the issue: some day we could change the functions to modify state. Actually, most of the changes in this PR are not needed. But I agree with your comment, so removing it.

apps/token-manager/contracts/TokenManager.sol Outdated
@@ -365,7 +365,7 @@ contract TokenManager is ITokenController, AragonApp { // ,IForwarder makes cove
* @param _amount The amount in the `approve()` call
* @return False if the controller does not authorize the approval
*/
function onApprove(address _owner, address _spender, uint _amount) public returns (bool) {
function onApprove(address _owner, address _spender, uint _amount) isInitialized public returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

apps/vault/contracts/Vault.sol Outdated
@@ -54,7 +54,7 @@ contract Vault is VaultBase {
connectors[ETH] = ethConnector;
}

function () payable public {
function () payable isInitialized public {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ignore the Vault for now, since we'll be changing it pretty soon.

@bingen bingen mentioned this pull request May 15, 2018
Merged
@bingen bingen merged commit 5c44f9b into master May 15, 2018
@sohkai sohkai deleted the issue303 branch May 15, 2018 08:37
bingen pushed a commit that referenced this pull request May 16, 2018
Finance: fix test
87d6ac8 
With changes in PR #307 (commit bb6a18d) sending would revert instead
of failing with invalid opcode, so last test needed to change from
`assertInvalidOpcode` to `assertRevert`. We didn't realize because CI was
broken prior to PR #309.
@bingen bingen mentioned this pull request May 16, 2018
Merged
MickdeGraaf pushed a commit to MickdeGraaf/aragon-apps that referenced this pull request Jan 28, 2020
Remove isInitialized mod from some functions
2064cca 
Addressing PR aragon#307 comments.
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
Remove isInitialized mod from some functions
a7b11bc 
Addressing PR aragon#307 comments.
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
@bingen
Merge pull request aragon#307 from aragon/issue303
b726cda 
Add isInitialized to all state-changing public functions
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
Finance: fix test
b402814 
With changes in PR aragon#307 (commit 770eaea) sending would revert instead
of failing with invalid opcode, so last test needed to change from
`assertInvalidOpcode` to `assertRevert`. We didn't realize because CI was
broken prior to PR aragon#309.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sohkai sohkai sohkai approved these changes

@izqui izqui Awaiting requested review from izqui

Assignees
No one assigned
Labels
audit: whg
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bingen @coveralls @sohkai