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.

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

Verify contracts using Solc standard JSON input #416

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion packages/buidler-core/test/builtin-tasks/flatten.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ describe("Flatten task", () => {

describe("When there no contracts", function() {
useFixtureProject("default-config-project");

it("should return empty string", async function() {
const flattenedFiles = await this.env.run(
TASK_FLATTEN_GET_FLATTENED_SOURCE
Expand All @@ -28,11 +29,22 @@ describe("Flatten task", () => {
describe("When has contracts", function() {
useFixtureProject("contracts-project");

it("should flattened files should sorted correctly", async function() {
it("should flatten files sorted correctly", async function() {
const flattenedFiles = await this.env.run(
TASK_FLATTEN_GET_FLATTENED_SOURCE
);
assert.deepEqual(getContractsOrder(flattenedFiles), ["C", "B", "A"]);
});
});

describe("When has contracts with name clash", function() {
useFixtureProject("contracts-nameclash-project");

it("should flatten files sorted correctly with repetition", async function() {
const flattenedFiles = await this.env.run(
TASK_FLATTEN_GET_FLATTENED_SOURCE
);
assert.deepEqual(getContractsOrder(flattenedFiles), ["C", "B", "A", "C"]);
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = {};
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pragma solidity ^0.5.1
import "./B.sol";
contract A {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pragma solidity ^0.5.1
import "./C.sol";
contract B {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pragma solidity ^0.5.1
contract C {};
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pragma solidity ^0.5.1
contract C {};
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ export interface EtherscanRequestParameters {
action: "verifysourcecode";
contractaddress: string;
sourceCode: string;
codeformat: string;
contractname: string;
compilerversion: string;
// The documentation is contradictory, but below is correct at this point in time (checked by experimentation).
// 1 = Optimizations used
// 0 = No optimizations used
optimizationUsed: number;
runs: number;
// optimizationUsed: number;
// runs: number;
// This is misspelt in Etherscan's actual API parameters.
// See: https://etherscan.io/apis#contracts
constructorArguements: string;
Expand All @@ -27,8 +28,8 @@ export function toRequest(params: {
sourceCode: string;
contractName: string;
compilerVersion: string;
optimizationsUsed: boolean;
runs: number;
// optimizationsUsed: boolean;
// runs: number;
constructorArguments: string;
libraries: string;
}): EtherscanRequestParameters {
Expand All @@ -38,10 +39,11 @@ export function toRequest(params: {
action: "verifysourcecode",
contractaddress: params.contractAddress,
sourceCode: params.sourceCode,
codeformat: "solidity-standard-json-input",
contractname: params.contractName,
compilerversion: params.compilerVersion,
optimizationUsed: params.optimizationsUsed ? 1 : 0,
runs: params.runs,
// optimizationUsed: params.optimizationsUsed ? 1 : 0,
// runs: params.runs,
constructorArguements: params.constructorArguments,
...parseLibraries(params.libraries)
};
Expand Down
18 changes: 6 additions & 12 deletions packages/buidler-etherscan/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {
TASK_COMPILE,
TASK_COMPILE_GET_COMPILER_INPUT,
TASK_FLATTEN_GET_FLATTENED_SOURCE
} from "@nomiclabs/buidler/builtin-tasks/task-names";
import { task } from "@nomiclabs/buidler/config";
Expand Down Expand Up @@ -46,30 +47,23 @@ task("verify-contract", "Verifies contract on etherscan")
);
}

let source: string;
try {
source = await run(TASK_FLATTEN_GET_FLATTENED_SOURCE);
} catch (_) {
throw new BuidlerPluginError(
`Your ${taskArgs.contractName} contract constains a cyclic dependency, Etherscan doesn't currently support contracts with such dependencies through its API, please use their GUI at https://etherscan.io/verifyContract to verify your contract.`
);
}

await run(TASK_COMPILE);
const abi = (await readArtifact(
config.paths.artifacts,
taskArgs.contractName
)).abi;
config.solc.fullVersion = await getLongVersion(config.solc.version);

const source = JSON.stringify(await run(TASK_COMPILE_GET_COMPILER_INPUT));

const request = toRequest({
apiKey: etherscan.apiKey,
contractAddress: taskArgs.address,
sourceCode: source,
contractName: taskArgs.contractName,
contractName: `contracts/${taskArgs.contractName}.sol:${taskArgs.contractName}`,
Copy link
Member

Choose a reason for hiding this comment

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

This line is the only problematic thing I can see, as it imposes the same names for files and contracts.

This could be fixed by doing something like

const solcInput : SolcInput = JSON.stringify(await run(TASK_COMPILE_GET_COMPILER_INPUT)); 

const filesWithContract = Object.entries(solcInput.sources).find((fileGlobalName, {content}) => hasContractDeclaration(content, taskArgs.contractName)).map((globalName) => globalName);

if (filesWithContract.length === 0) {
   // contract not found
   throw BuidlerPluginError(...);
}

if (filesWithContract.length > 1) {
   // throw error? what should be done here??
}

const contractName = `${filesWithContract[0}:${taskArgs.contractName}`;

Copy link
Contributor Author

@canepat canepat Feb 9, 2020

Choose a reason for hiding this comment

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

Yes, you're absolutely right, it's a limitation of this PR.

An idea for handling any use case is extending the verify-contract task arguments to accept an optional contractFilePath argument (e.g. contracts/A.sol) to build the complete contract name as ${taskArgs.contractFilePath}:${taskArgs.contractName}; when the contractFilePath is not given, the default simplified name contracts/${taskArgs.contractName}.sol:${taskArgs.contractName} will be used.

Another option could be changing the contractName argument of the verify-contract task to become the full contract name, specified exactly as expected by Etherscan API ${contractFilePath}:${contractName} (e.g. contracts/A.sol:B.sol).

IMHO the latter option is slightly preferable even if it changes a bit the semantic of the contractName argument of the verify-contract task.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@alcuadrado Does this issue gate merging? Thanks!

Copy link
Contributor Author

@canepat canepat Feb 26, 2020

Choose a reason for hiding this comment

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

@pcowgill this issue has been resolved in last commit 3695e1c

Copy link
Contributor

Choose a reason for hiding this comment

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

@canepat Great, thanks. I thought so but I wanted to confirm that

compilerVersion: config.solc.fullVersion,
optimizationsUsed: config.solc.optimizer.enabled,
runs: config.solc.optimizer.runs,
// optimizationsUsed: config.solc.optimizer.enabled,
// runs: config.solc.optimizer.runs,
constructorArguments: AbiEncoder.encodeConstructor(
abi,
taskArgs.constructorArguments
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module.exports = {
apiKey: process.env.ETHERSCAN_API_KEY || "testtoken"
},
solc: {
version: "0.5.1"
version: "0.5.15"
},
paths: {
artifacts: "artifacts-dir"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
pragma solidity 0.5.15;

contract ReentrancyGuard {
uint256 public guardCounter;

constructor() internal {
guardCounter = 1;
}

modifier nonReentrant() {
guardCounter += 1;
uint256 localCounter = guardCounter;
_;
require(localCounter == guardCounter, "re-entered");
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pragma solidity 0.5.1;
pragma solidity 0.5.15;

// import "./libraries/SafeMath.sol";
import "./TestContract1.sol";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pragma solidity 0.5.1;
pragma solidity 0.5.15;

contract TestContract1 {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
pragma solidity 0.5.15;

import "./imported/ReentrancyGuard.sol";

contract TestReentrancyGuardImported is ReentrancyGuard {

function foo() public nonReentrant returns(uint) {
return 0;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
pragma solidity 0.5.15;

import "./ReentrancyGuard.sol";

contract TestReentrancyGuardLocal is ReentrancyGuard {

function foo() public nonReentrant returns(uint) {
return 1;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
pragma solidity 0.5.15;

/**
* @title Helps contracts guard against reentrancy attacks.
* @author Remco Bloemen <remco@2π.com>, Eenae <alexey@mixbytes.io>
* @dev If you mark a function `nonReentrant`, you should also
* mark it `external`.
*/
contract ReentrancyGuard {
/// @dev counter to allow mutex lock with only one SSTORE operation
uint256 private _guardCounter;

constructor() internal {
// The counter starts at one to prevent changing it from zero to a non-zero
// value, which is a more expensive operation.
_guardCounter = 1;
}

/**
* @dev Prevents a contract from calling itself, directly or indirectly.
* Calling a `nonReentrant` function from another `nonReentrant`
* function is not supported. It is possible to prevent this from happening
* by making the `nonReentrant` function external, and make it call a
* `private` function that does the actual work.
*/
modifier nonReentrant() {
_guardCounter += 1;
uint256 localCounter = _guardCounter;
_;
require(localCounter == _guardCounter, "re-entered");
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
pragma solidity 0.5.1;
pragma solidity 0.5.15;


library SafeMath {
Expand Down
31 changes: 27 additions & 4 deletions packages/buidler-etherscan/test/integration/PluginTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { useEnvironment } from "../helpers";

// These are skipped because they can't currently be run in CI
describe.skip("Plugin integration tests", function() {
this.timeout(120000);
this.timeout(1000000);

describe("Using a correct Buidler project", () => {
useEnvironment(path.join(__dirname, "..", "buidler-project"));
Expand All @@ -32,16 +32,16 @@ describe.skip("Plugin integration tests", function() {
);
const amount = "20";

const deployedAddress = await deployContract(abi, `0x${bytecode}`, [
const deployedAddress = await deployContract(abi, `${bytecode}`, [
amount
]);

try {
await this.env.run("verify-contract", {
address: deployedAddress,
contractName: "TestContract",
contractName: "TestContract1",
libraries: JSON.stringify({
SafeMath: "0x292FFB096f7221c0C879c21535058860CcA67f58"
// SafeMath: "0x292FFB096f7221c0C879c21535058860CcA67f58"
}),
constructorArguments: [amount]
});
Expand All @@ -53,6 +53,29 @@ describe.skip("Plugin integration tests", function() {

return true;
});

it("Should verify deployed contract with name clash on etherscan", async function() {
await this.env.run(TASK_COMPILE, { force: false });

const { bytecode, abi } = await readArtifact(
this.env.config.paths.artifacts,
"TestReentrancyGuardLocal"
);
const deployedAddress = await deployContract(abi, `${bytecode}`, []);

try {
await this.env.run("verify-contract", {
address: deployedAddress,
contractName: "TestReentrancyGuardLocal",
libraries: JSON.stringify({}),
constructorArguments: []
});

assert.isTrue(true);
} catch (error) {
assert.fail(error.message);
}
});
});

describe("Using a Buidler project with circular dependencies", () => {
Expand Down
1 change: 1 addition & 0 deletions packages/buidler-solpp/test/js-config-project/.gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
/artifacts
/cache