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.

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

Disallow the use of TypeType in complex expressions #14347

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Disallow the use of TypeType in complex expressions #14347

merged 1 commit into from
Jul 18, 2023

Conversation

matheusaaguiar
Copy link
Collaborator

@matheusaaguiar matheusaaguiar commented Jun 22, 2023 •
edited by cameel
Loading

Fix #14319.
Depends on #14371. Merged.

@matheusaaguiar matheusaaguiar self-assigned this Jun 22, 2023
@matheusaaguiar matheusaaguiar force-pushed the fixTypeCheckingAbiDecode branch 2 times, most recently from f3f0f62 to 3705a9a Compare June 27, 2023 15:00
@matheusaaguiar
Copy link
Collaborator Author

@cameel , I added the test cases you suggested.

@cameel
Copy link
Member

cameel commented Jun 27, 2023

I added the test cases you suggested.

Well, something like this obviously won't work when used with abi.decode(), not sure it makes sense to test it:

abi.decode("", ((true ? addmod : addmod)(1, 2, 3)));

I posted these cases here because I thought you were going to include fixes for all the other broken cases in this PR (I specifically asked about this on the chat). Looks like it's not the case, and you'll be splitting it into more PRs - which is perfectly fine, but then you should include these in the PRs that will be fixing them, not here. Only the ones actually relevant to abi.decode() should be here.

@matheusaaguiar
Copy link
Collaborator Author

matheusaaguiar commented Jun 27, 2023 •
edited
Loading

I posted these cases here because I thought you were going to include fixes for all the other broken cases in this PR

Ah, it all makes sense now. I misunderstood when you asked about all the new cases. Now I see what you actually meant.
I read it as all the new cases for abi.decode, but it was in your PR for abi.decode. Sorry about that :)

I will remove that test. I guess the runtimeCode one too, right?

@cameel
Copy link
Member

cameel commented Jun 27, 2023

I will remove that test. I guess the runtimeCode one too, right?

Yeah. Only the cases with types make sense here.

@ekpyron ekpyron added this to the 0.8.21 milestone Jul 5, 2023
@matheusaaguiar matheusaaguiar changed the title Disallow complex expressions as argument of abi.decode Disallow the use of TypeType in complex expressions Jul 11, 2023
@matheusaaguiar matheusaaguiar changed the base branch from develop to overrideMagicType_mobileType July 11, 2023 13:39
@cameel cameel mentioned this pull request Jul 11, 2023
Merged
@matheusaaguiar matheusaaguiar added the has dependencies The PR depends on other PRs that must be merged first label Jul 12, 2023
cameel
cameel reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@cameel cameel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a few minor things remaining. Now the test coverage seems adequate.

But we need a changelog entry and a bug list entry. What's the plan here? Will this be done here, done in #14371 or done separately?

test/libsolidity/semanticTests/functionTypes/from_ternary_expression.sol Outdated Show resolved Hide resolved
test/libsolidity/syntaxTests/metaTypes/runtimeCode_from_ternary_with_type_expression.sol Outdated Show resolved Hide resolved
test/libsolidity/syntaxTests/metaTypes/type_expression_nested_type_max_keyword.sol Outdated Show resolved Hide resolved
@matheusaaguiar
Copy link
Collaborator Author

Just a few minor things remaining. Now the test coverage seems adequate.

Done.

But we need a changelog entry and a bug list entry. What's the plan here? Will this be done here, done in #14371 or done separately?

I added a changelog entry in 14371 that should cover both PRs. I think the bug list entry could go there too.

cameel
cameel previously approved these changes Jul 13, 2023
View reviewed changes
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

This is now ready, but we should be careful not to merge this before #14371. I usually keep dependent PRs as drafts to avoid that.

@cameel cameel mentioned this pull request Jul 13, 2023
Merged
@matheusaaguiar matheusaaguiar marked this pull request as draft July 14, 2023 08:30
@r0qs r0qs mentioned this pull request Jul 14, 2023
Closed
@cameel
Copy link
Member

cameel commented Jul 14, 2023

Needs rebase.

@matheusaaguiar
Copy link
Collaborator Author

Rebased.

@nikola-matic nikola-matic self-assigned this Jul 17, 2023
@cameel cameel assigned cameel and unassigned nikola-matic Jul 18, 2023
@cameel cameel force-pushed the overrideMagicType_mobileType branch from 0634543 to f3fc190 Compare July 18, 2023 08:56
Base automatically changed from overrideMagicType_mobileType to develop July 18, 2023 10:57
@cameel cameel dismissed their stale review July 18, 2023 10:57

The base branch was changed.

@cameel cameel marked this pull request as ready for review July 18, 2023 10:57
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Jul 18, 2023
@nikola-matic nikola-matic merged commit 6442741 into develop Jul 18, 2023
1 check passed
@nikola-matic nikola-matic deleted the fixTypeCheckingAbiDecode branch July 18, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikola-matic nikola-matic nikola-matic approved these changes

@cameel cameel cameel left review comments

@aarlt aarlt Awaiting requested review from aarlt

Assignees

@cameel cameel

@matheusaaguiar matheusaaguiar

Labels
None yet
Projects
Solidity Focus Board
Archived in project
Milestone
0.8.21
Development

Successfully merging this pull request may close these issues.

Fix type-checking for the type argument of abi.decode
4 participants
@matheusaaguiar @cameel @nikola-matic @ekpyron