Skip to content

[BREAKING] Revert commit and Make basefee a reserved identifier in 0.9.0 #11842

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 7, 2022
Merged

[BREAKING] Revert commit and Make basefee a reserved identifier in 0.9.0 #11842

merged 3 commits into from
Mar 7, 2022

Conversation

@v-sreekesh
Copy link
Contributor

@v-sreekesh v-sreekesh commented Aug 25, 2021 •
edited by hrkrshnn
Loading

Reverted the commit 7f1a2be and added the changes in changelog and rst files respectively

closes #11836

@v-sreekesh v-sreekesh mentioned this pull request Aug 25, 2021
Closed
hrkrshnn
hrkrshnn reviewed Aug 25, 2021
View reviewed changes
hrkrshnn
hrkrshnn reviewed Aug 25, 2021
View reviewed changes
hrkrshnn
hrkrshnn reviewed Aug 25, 2021
View reviewed changes
hrkrshnn
hrkrshnn reviewed Aug 25, 2021
View reviewed changes
@hrkrshnn
Copy link
Contributor

hrkrshnn commented Aug 25, 2021

The failing CI tests should be fixed by #11840.

axic
axic reviewed Aug 25, 2021
View reviewed changes
@cameel
Copy link
Collaborator

cameel commented Aug 25, 2021

Please rebase your PR on develop. It has fixes for the failing windows and pylint jobs.

In general it's best to ensure your branch is up to date with develop whenever you submit it for review or apply review fixes.

cameel
cameel reviewed Aug 25, 2021
View reviewed changes
docs/090-breaking-changes.rst Outdated
Comment on lines 18 to 19
- `error` is now a keyword and cannot be used as identifier anymore.
- Make basefee a reserved identifier in assembly.
Copy link
Collaborator

@cameel cameel Aug 25, 2021

Choose a reason for hiding this comment

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

Suggested change
- `error` is now a keyword and cannot be used as identifier anymore.
- Make basefee a reserved identifier in assembly.
- ``error`` is now a keyword and cannot be used as identifier anymore.
- Make ``basefee`` a reserved identifier in assembly.

Copy link
Collaborator

@cameel cameel Aug 25, 2021

Choose a reason for hiding this comment

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

@v-sreekesh Looks like the changes you made here have now disappeared.

I saw you tried to merge develop into your branch. This is not necessary. We have solved the problem in #11840 and once we merge it, you'll be able to rebase your PR on the updated breaking branch.

Copy link
Contributor Author

@v-sreekesh v-sreekesh Aug 25, 2021

Choose a reason for hiding this comment

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

Ok

@hrkrshnn
Copy link
Contributor

hrkrshnn commented Aug 25, 2021

Please rebase your PR on develop.

But this should go to breaking. But need to merge #11840 first.

hrkrshnn
hrkrshnn reviewed Aug 25, 2021
View reviewed changes
@cameel
Copy link
Collaborator

cameel commented Aug 25, 2021

But this should go to breaking.

Ah, you're totally right.

@v-sreekesh v-sreekesh force-pushed the reserve-basefee branch 3 times, most recently from a93c352 to 3176a42 Compare August 25, 2021 14:33
@hrkrshnn hrkrshnn marked this pull request as draft August 25, 2021 14:50
@v-sreekesh v-sreekesh force-pushed the reserve-basefee branch 3 times, most recently from 6d776b2 to 3176a42 Compare August 25, 2021 16:55
@cameel
Copy link
Collaborator

cameel commented Aug 25, 2021

Looks like bringing it up to date with develop has revealed some new errors.

  1. [BREAKING] Revert commit and Make basefee a reserved identifier in 0.9.0 #11842 (comment)
  2. test/libsolidity/semanticTests/inlineAssembly/basefee_berlin_function.sol needs to be moved to test/libsolidity/syntaxTests/ and updated because it now results in a compilation error. You can just move it and run isoltest with the --accept-updates to have the compiler update the expectations in the file for you.

Also, I see that the commits are now nicely squashed but there's still a bit of unnecessary stuff there: you can remove the empty merge commit from the branch (it does not add anything useful) and clean up the description of the remaining commit (it has some junk left over after squashing).

@v-sreekesh v-sreekesh marked this pull request as ready for review August 26, 2021 07:45
@v-sreekesh v-sreekesh requested review from axic, cameel and hrkrshnn August 26, 2021 07:45
@hrkrshnn hrkrshnn changed the title Revert commit and Make basfee as a reserved identifier in 0.9.0 [BREAKING] Revert commit and Make basfee as a reserved identifier in 0.9.0 Aug 26, 2021
@hrkrshnn
Copy link
Contributor

hrkrshnn commented Oct 4, 2021

I'll take over this PR.

@cameel cameel added the takeover Can be taken over by someone other than label giver label Oct 15, 2021
@cameel cameel changed the title [BREAKING] Revert commit and Make basfee as a reserved identifier in 0.9.0 [BREAKING] Revert commit and Make basefee as a reserved identifier in 0.9.0 Oct 27, 2021
@cameel cameel changed the title [BREAKING] Revert commit and Make basefee as a reserved identifier in 0.9.0 [BREAKING] Revert commit and Make basefee a reserved identifier in 0.9.0 Oct 27, 2021
@wechman wechman force-pushed the reserve-basefee branch 4 times, most recently from d07e82b to dd7e7cd Compare March 2, 2022 06:50
@wechman
Copy link
Contributor

wechman commented Mar 2, 2022

I have just updated tests and rebased changes on the newest breaking branch. The review can be resumed.

chriseth
chriseth reviewed Mar 2, 2022
View reviewed changes
Copy link
Contributor

@chriseth chriseth 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! Please squash the commits.

v-sreekesh and others added 3 commits March 2, 2022 10:48
@v-sreekesh @axic @wechman
Revert "Allow basefee as Yul identifier for EVMVersion < london"
9188e65 
This reverts commit 7f1a2be.

Added changes to changelog to include Make basfee as a reserved identifier

added changes under the breaking changes rst file

Update Changelog.md

Co-authored-by: Harikrishnan Mulackal <webmail.hari@gmail.com>

Update 090-breaking-changes.rst

moved the changes under "New restrictions" section

Update Changelog.md

avoided removing  line no 6

Update docs/090-breaking-changes.rst

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
@v-sreekesh @wechman
Added changes in changelog.md
5941f02
@wechman
Move basefee_berlin_function test to syntax tests.
8e2f929
@wechman
Copy link
Contributor

wechman commented Mar 2, 2022 •
edited
Loading

Looks good! Please squash the commits.

done

@leonardoalt leonardoalt merged commit 1d4cc6d into argotorg:breaking Mar 7, 2022
@Marenz Marenz mentioned this pull request Aug 30, 2022
Merged
Marenz added a commit that referenced this pull request Aug 30, 2022
@Marenz
Merge branch 'develop' into breaking
5663fbb 
Manual Resolved Conflicts:
	Changelog.md
	 * Updated changelog
	test/externalTests/ens.sh
	 * Merged fixes for upstream from both develop and breaking
	test/libsolidity/semanticTests/inlineAssembly/external_identifier_access_shadowing.sol
	 * Removed in #11735 (breaking)
	test/libsolidity/semanticTests/inlineAssembly/function_name_clash.sol
	 * Removed in #12209 (breaking)
	test/libsolidity/semanticTests/storage/mappings_array2d_pop_delete.sol
	 * Removed in #11843 (breaking)
	test/libsolidity/semanticTests/storage/mappings_array_pop_delete.sol
	 * Removed in #11843 (breaking)
	test/libsolidity/syntaxTests/inlineAssembly/basefee_berlin_function.sol
	 * Used version of file from #11842 (breaking)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@axic axic Awaiting requested review from axic

@hrkrshnn hrkrshnn Awaiting requested review from hrkrshnn

@cameel cameel Awaiting requested review from cameel

2 more reviewers

@chriseth chriseth chriseth left review comments

@leonardoalt leonardoalt leonardoalt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@wechman wechman

Labels

breaking change ⚠️ external contribution ⭐ takeover Can be taken over by someone other than label giver

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@v-sreekesh @hrkrshnn @cameel @wechman @axic @leonardoalt @chriseth