Skip to content

Disallow indexed reference types in events when using ABIEncoderV2 #4820

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.

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

Merged
merged 4 commits into from
Aug 15, 2018

Conversation

leonardoalt
Copy link
Member

Closes #4687

numIndexed++;
if (
_eventDef.sourceUnit().annotation().experimentalFeatures.count(ExperimentalFeature::ABIEncoderV2)
&& dynamic_cast<ReferenceType const*>(type(*var).get())
Copy link
Contributor

Choose a reason for hiding this comment

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

&& to the previous line

)
m_errorReporter.typeError(
var->location(),
"Reference types cannot be indexed."
Copy link
Contributor

Choose a reason for hiding this comment

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

Indexed reference types cannot yet be used with ABIEncoderV2

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.

Please add an endtoend test for dynamically-sized arrays both indexed and non-indexed and for both encoders (if supported) and a non-indexed struct with the new encoder.

@chriseth
Copy link
Contributor

Oh and the struct should be taken from storage.

@chriseth
Copy link
Contributor

Ah and I think we need an entry in the bug list don't we?

@leonardoalt leonardoalt force-pushed the disallow_indexed_ref_v2 branch from 337d63a to d6a8b94 Compare August 15, 2018 15:11
@leonardoalt leonardoalt force-pushed the disallow_indexed_ref_v2 branch from d6a8b94 to c00db3c Compare August 15, 2018 15:37
@codecov
Copy link

codecov bot commented Aug 15, 2018

Codecov Report

Merging #4820 into develop will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4820      +/-   ##
===========================================
+ Coverage    87.79%   87.82%   +0.03%     
===========================================
  Files          314      314              
  Lines        31299    31383      +84     
  Branches      3702     3702              
===========================================
+ Hits         27478    27562      +84     
  Misses        2561     2561              
  Partials      1260     1260
Flag Coverage Δ
#all 87.82% <100%> (+0.03%) ⬆️
#syntax 28.42% <14.28%> (-0.04%) ⬇️

@leonardoalt
Copy link
Member Author

@chriseth Let's merge this without the json path test and I'll create a separate PR for it.

@axic
Copy link
Member

axic commented Sep 26, 2018

I think this was working for strings, but not for structs.

Because the actual hashing is in done in ExpressionCompiler prior to abi encoding and that should work for strings. Well, it does use the specific "packed encoding" on it. Wonder if that is desired.

As it turned out in #5102 some tests rely on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants