Issue 678: Testing solidity strings with getters/setters #218

ccowell · 2018-11-07T18:25:25Z

Unable to confirm reported bug.

…h a shorter string with setters

… issue-678

eshaben

Looks good to me!

benjamincburns · 2018-11-07T21:38:57Z

Looks like a lot of commits for relatively small net change - I'd recommend squashing the commits and doing git push -f to keep the change log easier to read

benjamincburns · 2018-11-07T21:42:25Z

test/DynamicStringLength.sol

+
+contract DynamicStringLength {
+  string public testString;
+  uint public testId = 246;


looks like this & the event aren't used

Good catch.

benjamincburns · 2018-11-07T21:44:24Z

test/solidity_dynamic_strings.js

+const path = require("path");
+const compileAndDeploy = require("./helpers/contracts").compileAndDeploy;
+
+const mnemonic = "candy maple cake sugar pudding cream honey rich smooth crumble sweet treat";


The mnemonic defines the list of accounts that ganache will use - but you're using web3.eth.getAccounts below (which is good) so there's no need to hard code a mnemonic.

It's better if tests use randomized accounts - we want tests to look like best practice code examples.

Great suggestions for overall refactoring efforts.

benjamincburns · 2018-11-07T21:55:00Z

test/solidity_dynamic_strings.js

+}
+
+describe("Solidity variable string length", function() {
+  const context = setUp();


readability: since this is the only place the setUp function is called, I'm not sure what value it's providing. The usual flow for mocha tests is as shown in the snippet below, and this breaks that flow

describe(... describe( ... before(... it(... after(... } }

Great suggestions for overall refactoring efforts.

benjamincburns · 2018-11-07T21:55:32Z

test/solidity_dynamic_strings.js

+
+  before("setup web3", async function() {
+    context.provider = Ganache.provider(context.options);
+    context.options.blockTime = 2000;


I wouldn't set a block time - will just make the test slow

FWIW, only ever set block time if you're explicitly testing interval mining

benjamincburns · 2018-11-07T21:57:25Z

test/solidity_dynamic_strings.js

+  before("compile source", async function() {
+    this.timeout(10000);
+    context.contractArtifact = await compileAndDeploy(
+      path.join(__dirname, ".", `${contractName}.sol`),


no need to do __dirname, ".", contractFileName -- the dot is implicit - it's like saying /./contractName.sol which is the same as /contractName.sol

Great suggestions for overall refactoring efforts.

benjamincburns · 2018-11-07T22:00:07Z

test/solidity_dynamic_strings.js

+      contractName,
+      context.web3
+    );
+    context.instance = context.contractArtifact.instance;


unless you're using contractArtifact, I'd drop contractArtifact altogether and do

const { instance } = await compileAndDeploy( ... context.instance = instance

^ sweet ES6 destructuringing syntax will pull the instance field out of the object returned by compileAndDeploy and assign it to a const variable of the same name.

Great suggestions for overall refactoring efforts.

benjamincburns · 2018-11-07T22:47:46Z

test/solidity_dynamic_strings.js

+describe("Solidity variable string length", function() {
+  const context = setUp();
+  it("replacing a long string with a short string", async function() {
+    let response;


usually I only defined vars when they're first assigned unless for some unavoidable reason the variable needs to be accessed in a higher-level scope than the one in which it was assigned (which, in general, is a code smell)

Great suggestions for overall refactoring efforts.

benjamincburns

The actual test logic looks fine, but I'd recommend the following

Squash commits
Get rid of SetUp method
Don't set a block time (aka use instamining instead)
Don't specify a mnemonic (your test doesn't need deterministic accounts)
Add a test which is evaluated in the EVM

On the last one, I'd write another contract which actually checks the string assignment itself, and reverts if things go wrong. That is, it's possible that this is a bug in the EVM, and the final transaction state is fine, but during execution things are messed up.

Created a setup file for prepping test environments.

Added string validation at the EVM level

…a shorter string with setters Added support for basic multicontract compile and deployments Added string validation at the EVM level

… issue-678

nicholasjpaterno

LGTM! The only real concern I have is with the tests that may have been removed accidentally.

nicholasjpaterno · 2018-11-13T22:12:38Z

test/helpers/compile_deploy.js

+  const contractSources = selectedContracts.map((contractName) => {
+    return { [`${contractName}.sol`]: readFileSync(`${contractPath}${contractName}.sol`, "utf8") };
+  });
+  const sources = Object.assign(...contractSources);


I'm not sure we need this line here since this is the last usage of contractSources. If we do need a copy (in case compile mutates sources on line 19) I would change this to explicitly use an empty object as the target parameter Object.assign({}, ...contractSources) or just the spread operator like const sources = {...contractSources} and use the original contractSources in the return on line 40.

Agreed. Wanted to keep the code readable.

nicholasjpaterno · 2018-11-13T22:19:26Z

test/requests.js

@@ -505,22 +505,6 @@ const tests = function(web3) {
    });
  });

-  describe("eth_getTransactionReceipt", function() {


I think maybe this was removed by accident? This looks like it was part of @eshaben's commit for PR 212

This may be some squashing magic gone wrong

This was definitely from me

nicholasjpaterno · 2018-11-13T22:20:59Z

test/requests.js

@@ -661,11 +645,6 @@ const tests = function(web3) {
      assert(receipt.contractAddress, "should have deployed a contract");
    });

-    it("should return null for the to field due to contract creation (eth_getTransactionReceipt)", async function() {


Same as above ^ this may have been removed by accident from PR 212?

This may be some squashing magic gone wrong

nicholasjpaterno · 2018-11-13T22:23:03Z

test/solidity_dynamic_strings.js

+      let response = await instance.methods.testString().call();
+      assert.strictEqual(response, text);
+
+      // Set and confirm (within the EVM) a long string setting to a public variable


short* string

Are you suggesting to rename the variable? testString is being used to store a long string followed by a short string.

Simple fix 👍

nicholasjpaterno · 2018-11-13T22:44:27Z

test/solidity_dynamic_strings.js

@@ -0,0 +1,51 @@
+const assert = require("assert");
+const pretest = require("./helpers/pretest_setup");


We can destructure setup here.

I was opting for readability on this one. Fortunately this is the only export in pretest_setup for now.

I'll just rename setup to some thing more descriptive. 👍

ccowell · 2018-11-14T16:28:44Z

Going to create a new PR open with a cleaner branch.

Chris C added 9 commits November 6, 2018 19:14

Fixes #678: Confirmed that Solidity public strings can be updated wit…

e9bc6a3

…h a shorter string with setters

Reverted changes to request.js while testing out a eslint configuration

b91d55c

Fixes #678: Confirmed that Solidity public strings can be updated wit…

49ca041

…h a shorter string with setters

Reverted changes to request.js while testing out a eslint configuration

0469cc1

Merge branch 'issue-678' of github.com:trufflesuite/ganache-core into…

496a7ff

… issue-678

Added updated npm-shrinkwrap

56d3087

Resolves #678: Renamed test file to help avoid merge issues

d52dbf8

Adding renamed test file

1bfe4bc

Removed unneeded 'new' from Ganache provider instance

82be61b

ccowell requested a review from eshaben November 7, 2018 20:08

eshaben approved these changes Nov 7, 2018

View reviewed changes

benjamincburns reviewed Nov 7, 2018

View reviewed changes

benjamincburns suggested changes Nov 7, 2018

View reviewed changes

Chris C added 9 commits November 8, 2018 16:23

Cleaned up unnecessary code and add some es6 reactoring.

c104398

Created a setup file for prepping test environments.

Destructured accounts and contract instance

9335d3a

Add set confirmation to the contract

4e80e6f

Added support for basic multicontract compile and deployments

96746d9

Added string validation at the EVM level

Cleaned up a couple of function interfaces

153e82b

Updated shrinkwrap.

62716c2

Merge branch 'develop' into issue-678

188abbd

Fixes #678: Confirmed that Solidity public strings can be updated to …

47268a4

…a shorter string with setters Added support for basic multicontract compile and deployments Added string validation at the EVM level

Merge branch 'issue-678' of github.com:trufflesuite/ganache-core into…

c14838e

… issue-678

nicholasjpaterno self-requested a review November 13, 2018 22:02

nicholasjpaterno reviewed Nov 13, 2018

View reviewed changes

ccowell closed this Nov 14, 2018

ccowell deleted the issue-678 branch November 14, 2018 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 678: Testing solidity strings with getters/setters #218

Issue 678: Testing solidity strings with getters/setters #218

ccowell commented Nov 7, 2018

eshaben left a comment

benjamincburns commented Nov 7, 2018

benjamincburns Nov 7, 2018

ccowell Nov 8, 2018

benjamincburns Nov 7, 2018

ccowell Nov 8, 2018

benjamincburns Nov 7, 2018

ccowell Nov 8, 2018

benjamincburns Nov 7, 2018

benjamincburns Nov 7, 2018

benjamincburns Nov 7, 2018

ccowell Nov 8, 2018

benjamincburns Nov 7, 2018 •

edited

Loading

ccowell Nov 8, 2018

benjamincburns Nov 7, 2018 •

edited

Loading

ccowell Nov 8, 2018

benjamincburns left a comment

nicholasjpaterno left a comment

nicholasjpaterno Nov 13, 2018 •

edited

Loading

ccowell Nov 13, 2018

nicholasjpaterno Nov 13, 2018

ccowell Nov 13, 2018

eshaben Nov 13, 2018

nicholasjpaterno Nov 13, 2018

ccowell Nov 13, 2018

nicholasjpaterno Nov 13, 2018

ccowell Nov 13, 2018

ccowell Nov 13, 2018

nicholasjpaterno Nov 13, 2018

ccowell Nov 13, 2018

ccowell Nov 14, 2018

ccowell commented Nov 14, 2018

		@@ -0,0 +1,51 @@
		const assert = require("assert");
		const pretest = require("./helpers/pretest_setup");

Issue 678: Testing solidity strings with getters/setters #218

Issue 678: Testing solidity strings with getters/setters #218

Conversation

ccowell commented Nov 7, 2018

eshaben left a comment

Choose a reason for hiding this comment

benjamincburns commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamincburns Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamincburns Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamincburns left a comment

Choose a reason for hiding this comment

nicholasjpaterno left a comment

Choose a reason for hiding this comment

nicholasjpaterno Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccowell commented Nov 14, 2018

benjamincburns Nov 7, 2018 •

edited

Loading

benjamincburns Nov 7, 2018 •

edited

Loading

nicholasjpaterno Nov 13, 2018 •

edited

Loading