Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Validate constructors in ancestor contracts #1385

Merged
merged 5 commits into from
Jan 14, 2020

Conversation

spalladino
Copy link
Contributor

Fixes #1332

@spalladino spalladino force-pushed the fix/inherited-constructor-#1332 branch from 7d03411 to 5718f16 Compare January 14, 2020 00:00
@spalladino
Copy link
Contributor Author

spalladino commented Jan 14, 2020

Funny thing. The GSNRecipient from contracts-ethereum-package extends from Context, which has an internal constructor.

And apparently Solidity generates an ABI entry for the constructor (even if it's internal), which is being picked up by the new validation, and causes the integration test to fail.

$ jq .abi build/contracts/Context.json 
[{
    "inputs": [],
    "payable": false,
    "stateMutability": "nonpayable",
    "type": "constructor"
}]

From the test log:

  1) Unpack a GSN Kit on the development
       Create a Counter proxy on the development:
     Error: Error running npx oz create Counter --init initialize --args 2 --network development (err 1)
undefined
- Compiling contracts with Truffle, using settings from truffle.js file
- Compiling contracts with Truffle, using settings from truffle.js file
- Compiling contracts with Truffle, using settings from truffle.js file
- - Contract Counter or an ancestor has a constructor. Change it to an initializer function. See https://docs.openzeppelin.com/sdk/2.6/writing-contracts#initializers.
- One or more contracts have validation errors. Please review the items listed above and fix them, or run this command again with the --force option.

      at execInWorkdir (/home/circleci/openzeppelin/tests/util/share.js:62:13)
      at run (/home/circleci/openzeppelin/tests/util/share.js:78:24)
      at Context.<anonymous> (src/integration.test.js:68:7)

We'll have to think how to get around this. Perhaps adding an exception for the Context contract? Or checking if the ctor body is empty?

@spalladino spalladino added the status:blocked Blocked by another issue label Jan 14, 2020
@spalladino spalladino added status:to-review Awaiting review and removed status:blocked Blocked by another issue labels Jan 14, 2020
@@ -12,6 +12,6 @@ export function hasConstructor(contract: Contract, buildArtifacts: BuildArtifact
function hasNonEmptyConstructorInAST(contractNode: any): boolean {
return contractNode.nodes
.filter((n: any) => n.nodeType === 'FunctionDefinition' && n.kind === 'constructor')
.filter((n: any) => n.body.statements.length > 0)
.filter((n: any) => n.body.statements.length > 0 || n.modifiers.length > 0)
Copy link
Contributor

@frangio frangio Jan 14, 2020

Choose a reason for hiding this comment

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

There can be false positives if there is a "modifier" that is a super call to a parent contract with an empty constructor. But that's an edge case that I don't think will happen.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Looks good! There's linter errors though.

@spalladino spalladino added status:ready-to-merge Order mergify to merge and removed status:to-review Awaiting review labels Jan 14, 2020
@mergify mergify bot merged commit 219a7f5 into master Jan 14, 2020
@mergify mergify bot deleted the fix/inherited-constructor-#1332 branch January 14, 2020 23:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:ready-to-merge Order mergify to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK fails to identify inherited constructors with no arguments
2 participants