Skip to content

Conversation

@axic
Copy link
Contributor

@axic axic commented Sep 26, 2018

Closes #4700.

@axic axic changed the title Add CLI option to soltest to always use ABIEncoderV2 Run all end-to-end tests with ABIEncoderV2 Sep 26, 2018
@axic axic force-pushed the abiencoderv2-tests branch from f619776 to 1117488 Compare September 26, 2018 17:19
@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #5102 into develop will decrease coverage by 60.53%.
The diff coverage is 4.16%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #5102       +/-   ##
============================================
- Coverage    88.33%   27.79%   -60.54%     
============================================
  Files          362      361        -1     
  Lines        34864    34684      -180     
  Branches      4131     4136        +5     
============================================
- Hits         30796     9640    -21156     
- Misses        2693    24370    +21677     
+ Partials      1375      674      -701
Flag Coverage Δ
#all ?
#syntax 27.79% <4.16%> (-0.01%) ⬇️

@axic
Copy link
Contributor Author

axic commented Sep 26, 2018

It seems not all the tests can be compiled with it:

:2:1: Warning: Experimental features are turned on. Do not use experimental features on live deployments.
pragma experimental ABIEncoderV2;
^-------------------------------^
:76:2: Warning: This declaration shadows an existing declaration.
	function Registrar() public {
 ^ (Relevant source part starts here and spans across multiple lines).
:11:1: The shadowed declaration is here:
contract Registrar is NameRegister {
^ (Relevant source part starts here and spans across multiple lines).

:12:16: Error: Indexed reference types cannot yet be used with ABIEncoderV2.
	event Changed(string indexed name);
	              ^-----------------^
:13:23: Error: Indexed reference types cannot yet be used with ABIEncoderV2.
	event PrimaryChanged(string indexed name, address indexed addr);
	                     ^-----------------^
:24:21: Error: Indexed reference types cannot yet be used with ABIEncoderV2.
	event AuctionEnded(string indexed _name, address _winner);
	                   ^------------------^
:25:15: Error: Indexed reference types cannot yet be used with ABIEncoderV2.
	event NewBid(string indexed _name, address _bidder, uint _value);
	             ^------------------^

Is that an actual bug we have or the old encoder supports it?

Note, the tests don't care about those events, so they may as well be broken.

Update: there is a single test, event_indexed_string, which does seem to validate it.

@axic
Copy link
Contributor Author

axic commented Sep 26, 2018

So it seems all the failures are for cases where the two encoders explicitly differ.

Good news is that every other case works 🎉

Now need to figure out a way how to properly have this merged.

@axic axic force-pushed the abiencoderv2-tests branch from 10f7a82 to 34f23be Compare November 21, 2018 19:19
@axic
Copy link
Contributor Author

axic commented Nov 21, 2018

@chriseth @ekpyron do you think this can be finished easily (i.e. certain tests have to have the ability to say they are not ABIEncoderV2 compatible)?

If not, should we merge the soltest command line option so that locally it can be run, but not make it part of the CI?

Or should we just close this.

@chriseth
Copy link
Contributor

I think this should be a feature of the new extracted end to end test format.

Another option would be to just implement indexed reference parameters for events for abi encoder v2.

@chriseth
Copy link
Contributor

Rebased (and removed the "indexed" modifications).

@chriseth
Copy link
Contributor

chriseth commented Feb 18, 2019

It looks like after #6000 and #6029 the only problem left is something to do with a cleanup function for mappings - and looking at the code, this might actually be already fixed.

@chriseth
Copy link
Contributor

chriseth commented Feb 18, 2019

I was wrong, needed #6030.

After that, this should turn green (apart from the few tests that are specifically written to highlight differences in the two encoders).

@chriseth
Copy link
Contributor

Rebased on top of the fixes.

@axic axic force-pushed the abiencoderv2-tests branch from a5eb384 to 8ace2c5 Compare February 21, 2019 11:20
@chriseth chriseth merged commit 27d936c into develop Feb 21, 2019
@axic axic deleted the abiencoderv2-tests branch February 21, 2019 12:11
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