[BREAKING] Drop constant and payable from ABI. #4090

ekpyron · 2018-05-08T11:12:59Z

Closes #4755.

Part of #3656.

To be discussed:

How to deal with breaking changes and the contracts in test/compilationTests/?
Does it make sense to merge changes like this to the 050 branch at this point?

ekpyron · 2018-05-08T11:25:13Z

One of the reasons to create this was to see how the tests behave and start a discussion about that.
As expected, the tests involve external contracts, e.g. from truffle and the DAO, that are not yet forward compatible with changes like this, so we have to decide how to deal with this.

axic · 2018-05-08T14:59:31Z

@ekpyron can we merge the constant -> view changes (docs, styleguide, std, test) separately on develop? That it not breaking.

ekpyron · 2018-05-08T15:05:16Z

@axic That's true - I have them in separate commits here, so I'll extract them into a separate PR shortly!

ekpyron · 2018-05-08T15:45:56Z

Once #4093 is merged to develop and develop is merged into 050, I'll rebase this PR.

libsolidity/interface/ABI.cpp

axic · 2018-05-09T13:30:06Z

The ViewPureChecker must be updated to be turned on for view (there's a 0.5.0 check there).

ekpyron · 2018-05-11T09:05:36Z

@axic The two last commits now enable enforcing view in ViewPureChecker. The last commit merely adjusts some end-to-end-tests and I extracted it to #4093 as well.
Actually I would have considered enabling these errors to be separate from this PR, but I guess it makes sense to add it in here.

ekpyron · 2018-05-11T09:25:03Z

Of course, since this last change (ViewPureChecker reports errors for view), the tests will fail again and are much more involved to update. Since we decided to disable the compilation tests for 0.5.0 I will not update them any further.

ekpyron · 2018-05-14T12:29:53Z

Rebased and removed the commit that updates test/compilationTests (since we decided to disable those tests for 0.5.0).

docs/abi-spec.rst

chriseth · 2018-05-14T20:54:56Z

libsolidity/analysis/ViewPureChecker.cpp

-		for (ContractDefinition const* c: source->filteredNodes<ContractDefinition>(source->nodes()))
-			contracts[c] = enforceView;
+		auto const& sourceContracts = source->filteredNodes<ContractDefinition>(source->nodes());
+		contracts.insert(contracts.end(), sourceContracts.begin(), sourceContracts.end());


Can you use contracts = source->filteredNodes<ContractDefinition>(source->nodes())?

Didn't know that that is defined for std::vector in libdevcore - I'll change it.

chriseth · 2018-05-14T20:56:47Z

test/contracts/LLL_ERC20.cpp

@@ -216,39  216,39 @@ static char const* erc20Code = R"DELIMITER(

      ;; ----------------------------------------------------------------------
      ;; Getter for the name of the token.
-      ;; @abi name() constant returns (string)


This is lll, do we really want to change it?

chriseth · 2018-05-14T20:57:13Z

test/libsolidity/ASTLegacyJSON.cpp

@@ -266,15  266,13 @@ BOOST_AUTO_TEST_CASE(function_type)
 	BOOST_CHECK_EQUAL(argument["attributes"]["name"], "x");
 	BOOST_CHECK_EQUAL(argument["attributes"]["type"], "function () payable external returns (uint256)");
 	Json::Value funType = argument["children"][0];
-	BOOST_CHECK_EQUAL(funType["attributes"]["constant"], false);


Can you add the corresponding check about stateMutability?

axic · 2018-08-01T10:53:48Z

@ekpyron can you create a PR with the AST changes only?

ekpyron · 2018-08-01T11:05:58Z

@axic I will.

axic · 2018-08-01T11:09:52Z

docs/abi-spec.rst

 - ``constant``: ``true`` if function is either ``pure`` or ``view``, ``false`` otherwise.

-``type`` can be omitted, defaulting to ``"function"``, likewise ``payable`` and ``constant`` can be omitted, both defaulting to ``false``.
+.. warning::


See the warning below, which has the same content.

codecov · 2018-08-02T16:36:32Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@b0a2e41). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             develop    #4090    /-   ##
==========================================
  Coverage           ?   28.31%           
==========================================
  Files              ?      313           
  Lines              ?    30650           
  Branches           ?     3662           
==========================================
  Hits               ?     8678           
  Misses             ?    21301           
  Partials           ?      671

Flag	Coverage Δ
#syntax	`28.31% <0%> (?)`

iurimatias · 2018-08-02T16:39:28Z

@axic we do and it's a breaking change for us, we'll need to update

axic · 2018-08-08T12:41:49Z

@chriseth @ekpyron I think we can ~~close this now~~ postpone this now. Since I consider this a "protocol change", it doesn't necessarily needs to be aligned with breaking releases. Just the time has to be right for the ecosystem to adapt.

ekpyron · 2018-08-08T13:39:02Z

@axic Yes, I think we were pretty much heading towards postponing this anyways and classifying this as protocol change and decoupling those from breaking releases makes sense.

chriseth · 2018-08-15T08:27:11Z

Removed from 0.5.0.

axic

Needs to be rebased.

Rebased. The question is whether we consider this a "breaking change" for 0.6.0 or an "ecosystem/interface change" the we can do before that.

axic · 2018-11-14T12:33:57Z

It is missing a changelog entry and all the tests are failing.

# tmp

ekpyron · 2018-12-13T11:28:04Z

Easy to recreate and unclear when we'll actually do this, so I'm closing this PR for now and added the issue to the backlog.

ekpyron self-assigned this May 8, 2018

ekpyron mentioned this pull request May 8, 2018

Prepare drop constant keyword. #4093

Merged

axic reviewed May 9, 2018

View reviewed changes

libsolidity/interface/ABI.cpp Show resolved Hide resolved

chriseth force-pushed the 050 branch from dc857fe to 40cc63c Compare May 14, 2018 12:04

ekpyron force-pushed the dropConstantKeyword branch 2 times, most recently from 5ff0e91 to ebd020d Compare May 14, 2018 12:28

ekpyron force-pushed the dropConstantKeyword branch 5 times, most recently from f4f8f1f to 2d43635 Compare May 14, 2018 18:44

chriseth reviewed May 14, 2018

View reviewed changes

docs/abi-spec.rst Outdated Show resolved Hide resolved

chriseth previously requested changes May 14, 2018

View reviewed changes

chriseth force-pushed the 050 branch from f2ff592 to 07acc22 Compare May 15, 2018 08:56

ekpyron force-pushed the dropConstantKeyword branch 3 times, most recently from 58e64a3 to 7c0f44f Compare May 15, 2018 12:48

chriseth force-pushed the 050 branch from 2d1ffcf to f98c8fa Compare May 15, 2018 13:08

ekpyron force-pushed the dropConstantKeyword branch from 7c0f44f to 09e0816 Compare May 15, 2018 13:15

ekpyron changed the title ~~[WIP, BREAKING] Drop constant keyword.~~ [BREAKING] Drop constant keyword. May 15, 2018

ekpyron force-pushed the dropConstantKeyword branch from fb6f033 to fb73042 Compare August 1, 2018 08:47

axic reviewed Aug 1, 2018

View reviewed changes

ekpyron force-pushed the dropConstantKeyword branch from fb73042 to 5ebfcd4 Compare August 2, 2018 16:36

ekpyron mentioned this pull request Aug 2, 2018

[BREAKING] Drop constant field from JSON AST #4663

Merged

ekpyron force-pushed the dropConstantKeyword branch from 5ebfcd4 to 6805900 Compare August 8, 2018 11:30

ekpyron changed the title ~~[BREAKING] Drop constant from ABI.~~ [BREAKING] Drop constant and payable from ABI. Aug 8, 2018

ekpyron removed their assignment Sep 14, 2018

axic previously requested changes Nov 14, 2018

View reviewed changes

ekpyron force-pushed the dropConstantKeyword branch from 6805900 to 4415541 Compare November 14, 2018 12:02

ekpyron force-pushed the dropConstantKeyword branch from 4415541 to 9a6adf8 Compare November 14, 2018 12:50

Remove constant and payable from ABI.

aff0e33

# tmp

ekpyron force-pushed the dropConstantKeyword branch from 9a6adf8 to 6e5de3b Compare November 14, 2018 13:00

ekpyron added 2 commits November 14, 2018 14:25

Update test suite.

d9b1b43

Adjust abi-spec doc.

361d1b9

ekpyron force-pushed the dropConstantKeyword branch from 6e5de3b to 361d1b9 Compare November 14, 2018 13:25

ekpyron closed this Dec 13, 2018

ekpyron deleted the dropConstantKeyword branch May 8, 2019 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BREAKING] Drop constant and payable from ABI. #4090

[BREAKING] Drop constant and payable from ABI. #4090

ekpyron commented May 8, 2018 •

edited by axic

Loading

ekpyron commented May 8, 2018

axic commented May 8, 2018 •

edited

Loading

ekpyron commented May 8, 2018

ekpyron commented May 8, 2018 •

edited

Loading

axic commented May 9, 2018

ekpyron commented May 11, 2018

ekpyron commented May 11, 2018

ekpyron commented May 14, 2018

chriseth May 14, 2018

ekpyron May 15, 2018

chriseth May 14, 2018

chriseth May 14, 2018

ekpyron May 15, 2018

axic commented Aug 1, 2018

ekpyron commented Aug 1, 2018

axic Aug 1, 2018

codecov bot commented Aug 2, 2018 •

edited

Loading

iurimatias commented Aug 2, 2018

axic commented Aug 8, 2018 •

edited

Loading

ekpyron commented Aug 8, 2018

chriseth commented Aug 15, 2018

axic left a comment

axic commented Nov 14, 2018

ekpyron commented Dec 13, 2018

[BREAKING] Drop constant and payable from ABI. #4090

[BREAKING] Drop constant and payable from ABI. #4090

Conversation

ekpyron commented May 8, 2018 • edited by axic Loading

ekpyron commented May 8, 2018

axic commented May 8, 2018 • edited Loading

ekpyron commented May 8, 2018

ekpyron commented May 8, 2018 • edited Loading

axic commented May 9, 2018

ekpyron commented May 11, 2018

ekpyron commented May 11, 2018

ekpyron commented May 14, 2018

chriseth May 14, 2018

Choose a reason for hiding this comment

ekpyron May 15, 2018

Choose a reason for hiding this comment

chriseth May 14, 2018

Choose a reason for hiding this comment

chriseth May 14, 2018

Choose a reason for hiding this comment

ekpyron May 15, 2018

Choose a reason for hiding this comment

axic commented Aug 1, 2018

ekpyron commented Aug 1, 2018

axic Aug 1, 2018

Choose a reason for hiding this comment

codecov bot commented Aug 2, 2018 • edited Loading

Codecov Report

iurimatias commented Aug 2, 2018

axic commented Aug 8, 2018 • edited Loading

ekpyron commented Aug 8, 2018

chriseth commented Aug 15, 2018

axic left a comment

Choose a reason for hiding this comment

axic commented Nov 14, 2018

ekpyron commented Dec 13, 2018

ekpyron commented May 8, 2018 •

edited by axic

Loading

axic commented May 8, 2018 •

edited

Loading

ekpyron commented May 8, 2018 •

edited

Loading

codecov bot commented Aug 2, 2018 •

edited

Loading

axic commented Aug 8, 2018 •

edited

Loading