[BREAKING] Drop constant and payable from ABI. by ekpyron · Pull Request #4090 · argotorg/solidity

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


      ;; ----------------------------------------------------------------------
      ;; 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

 	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?

ekpyron · 2018-05-15T14:22:26Z

@chriseth Should we maybe disable the compilation tests on 050 already?

axic · 2018-07-23T18:22:46Z

docs/abi-spec.rst

+
+.. warning::
+    Prior to version 0.5.0 there was an additional field ``constant``. In 0.5.0 this field has been removed.
+    Instead you can check whether ``stateMutability`` is ``pure`` or ``view``.


We should create a PR adding a deprecation warning for both constant and payable.

Did that in #4549.

axic · 2018-07-23T18:33:04Z

This PR changes both the AST JSON and the ABI output. I think we can merge the AST JSON change, but as agreed the ABI should remain a little bit longer.

axic · 2018-07-25T12:02:45Z

We should also submit a PR to go-ethereum's abigen and potentially parity's abigen.

Truffle 5.x was updated and they plan to backport the changes 4.x. @frozeman made web3 releases.

@yann300 @iurimatias is remix / embark making use of 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

chriseth force-pushed the 050 branch from f98c8fa to cb4a9ab Compare May 15, 2018 15:06

ekpyron force-pushed the dropConstantKeyword branch 2 times, most recently from 8e04dd3 to fb6f033 Compare July 13, 2018 10:39

axic reviewed Jul 23, 2018

View reviewed changes

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

Conversation

ekpyron commented May 8, 2018 • edited by axic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekpyron commented May 8, 2018

Uh oh!

axic commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekpyron commented May 8, 2018

Uh oh!

ekpyron commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

axic commented May 9, 2018

Uh oh!

ekpyron commented May 11, 2018

Uh oh!

ekpyron commented May 11, 2018

Uh oh!

ekpyron commented May 14, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axic commented Jul 23, 2018

Uh oh!

axic commented Jul 25, 2018

Uh oh!

axic commented Aug 1, 2018

Uh oh!

ekpyron commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

iurimatias commented Aug 2, 2018

Uh oh!

axic commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekpyron commented Aug 8, 2018

Uh oh!

chriseth commented Aug 15, 2018

Uh oh!

axic left a comment

Choose a reason for hiding this comment

Uh oh!

axic commented Nov 14, 2018

Uh oh!

ekpyron commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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