Refactor ExtVMFace #4496

chfast · 2017-09-13T14:05:54Z

This moves utility structs like LogEntry from ExtVMFace.h (libevm) to libethcore. Previously some files from libethcore have to include ExtVMFace.h.

Also Address is moved from libdevcrypto to libdevcore as it is used not only in crypto context.

winsvega · 2017-09-13T17:08:36Z

@pirapira

gumb0 · 2017-09-15T09:44:31Z

libdevcore/FixedHash.cpp

+namespace dev
+{
+
+const Address ZeroAddress = Address();


Address const

gumb0 · 2017-09-15T09:45:07Z

libdevcore/FixedHash.cpp

-/** @file FixedHash.cpp
- * @author Gav Wood <[email protected]>
- * @date 2014
- */

 #include "FixedHash.h"
 #include <boost/algorithm/string.hpp>

 using namespace std;
 using namespace dev;


I think we can remove this now

gumb0 · 2017-09-15T09:48:21Z

libdevcore/FixedHash.h

@@ -351,6 +351,25 @@ using h160Set = std::set<h160>;
 using h256Hash = std::unordered_set<h256>;
 using h160Hash = std::unordered_set<h160>;

+/// An Ethereum address: 20 bytes.
+/// @NOTE This is not endian-specific; it's just a bunch of bytes.
+using Address = h160;


I'd prefer to put these into separate Address.h/cpp

Two cons for this: increased build time (?) and additional maintenance cost. I'm not sure...

I think it's your way it could be theoretically increased build time, because everyone who needs only FixedHash has to compile Address-related stuff, too

It would make navigating the code easier, putting Address into Address.h instead of suprisingly FixedHash.h

gumb0 · 2017-09-15T09:56:13Z

libethcore/Transaction.h


 #pragma once

+#include <boost/optional.hpp>


#includes should be in increasing order of generality, so this goes after our own headers

I think we have it the other way around in most files. Personally, I don't care. Why is this better?

Link from coding standards: http://stackoverflow.com/questions/614302/c-header-order/614333#614333

It prevents the headers from not including all necessary headers but still working because of weird inter-header dependency

gumb0 · 2017-09-15T09:57:28Z

libethereum/TransactionReceipt.h

 #include <boost/variant/variant.hpp>
+#include <libdevcore/RLP.h>


should be before boost & <array>

gumb0 · 2017-09-15T10:00:49Z

libethcore/Common.h

+	Live
+};
+
+struct LogEntry


These could go into separate LogEntry.h, but it's up to you

pirapira · 2017-09-15T17:58:17Z

libethcore/BasicAuthority.h

@@ -23,6 +23,7 @@

 #pragma once

+#include <libdevcrypto/Common.h>


Why is this added now?

Previously this header was implicitly included for Address. Now we still need it for crypto::Signature.

pirapira · 2017-09-15T17:59:13Z

libethcore/Common.cpp

@@ -191,5 +189,26 @@ string TransactionSkeleton::userReadable(bool _toProxy, function<pair<bool, stri
 		formatBalance(gas * gasPrice) + ".");
 }

+LogEntry::LogEntry(RLP const& _r)
+{


Should there be an assertion against

_r not being a list and

_r being too short?

pirapira · 2017-09-15T18:02:43Z

libethcore/Common.h

+{
+	LogEntry() = default;
+	explicit LogEntry(RLP const& _r);
+	LogEntry(Address const& _address, h256s _ts, bytes&& _d): address(_address), topics(std::move(_ts)), data(std::move(_d)) {}


I wonder why _ts is not an rvalue reference and _d is an rvalue reference...

Generic way is to pass by value and move in the initialization list (like with h256s case). This way you handle both copy and move, depending what argument you have.

The version with && prevents you from passing argument by copy -- you can only pass an argument being rvalue. This is restrictive, but enforces some optimizations. I don't like it because it is more confusing than useful.

codecov-io · 2017-09-20T09:31:38Z

Codecov Report

Merging #4496 into develop will increase coverage by 6.4%.
The diff coverage is 82.92%.

@@            Coverage Diff            @@
##           develop   #4496     +/-   ##
=========================================
+ Coverage    56.29%   62.7%   +6.4%     
=========================================
  Files          337     368     +31     
  Lines        22788   30991   +8203     
  Branches      2750    2749      -1     
=========================================
+ Hits         12829   19433   +6604     
- Misses        9090   11321   +2231     
+ Partials       869     237    -632

Impacted Files	Coverage Δ
libethereum/BlockChain.h	`72.91% <ø> (-27.09%)`	⬇️
libethcore/Common.h	`90% <ø> (ø)`
test/tools/jsontests/vm.cpp	`62.7% <ø> (+4.18%)`	⬆️
libdevcrypto/Common.cpp	`89.37% <ø> (ø)`
libweb3jsonrpc/Eth.h	`20% <ø> (+20%)`	⬆️
libethcore/SealEngine.cpp	`82.35% <ø> (+9.62%)`	⬆️
test/tools/libtesteth/TestHelper.h	`40% <ø> (+15%)`	⬆️
libweb3jsonrpc/AccountHolder.h	`84.61% <ø> (+9.61%)`	⬆️
libevm/ExtVMFace.h	`74.46% <ø> (+9.46%)`	⬆️
libethereum/Transaction.h	`50% <ø> (+25%)`	⬆️
... and 287 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75be259...b4fc3aa. Read the comment docs.

chfast · 2017-09-20T10:57:08Z

I think I addressed all issues. Can you review before builds?

gumb0 · 2017-09-20T11:01:36Z

libethcore/Common.h

@@ -90,6 +96,13 @@ enum class RelativeBlock: BlockNumber
 	Pending = PendingBlock
 };

+enum class BlockPolarity


This is used only in LocalisedLogEntry, maybe move it to LogEntry.h?

Unfortunately, this is also used in Client.cpp and ClientBase.cpp.

chfast added the in progress label Sep 13, 2017

chfast force-pushed the refactor-extvmface branch from b027ab6 to 145c0d5 Compare September 13, 2017 14:12

winsvega requested a review from pirapira September 13, 2017 17:08

chfast added needs review and removed in progress labels Sep 13, 2017

chfast requested a review from gumb0 September 14, 2017 13:06

chfast force-pushed the refactor-extvmface branch from 145c0d5 to f55c3c1 Compare September 14, 2017 16:16

gumb0 suggested changes Sep 15, 2017

View reviewed changes

pirapira reviewed Sep 15, 2017

View reviewed changes

chfast added 8 commits September 20, 2017 10:27

Move Address from devcrypto to devcore

0b86baa

Move Address from libdevcrypto to libdevcore

8350af7

Move BlockPolarity from ExtVMFace.h to libethcore

a08d5b7

Refactor LogEntries

268d56d

Move LogEntry from ExtVMFace.h to libethcore

23ca17d

Do not include ExtVMFace.h if not needed

b819cdf

Add assert protecting LogEntry(RLP) constructor

5a0016b

Move Address to separate module

f4ec266

ethereum deleted a comment from pirapira Sep 20, 2017

chfast added 3 commits September 20, 2017 11:00

Sort includes in some recently modified files

878e721

LogEntry: Clean up the constructor

c42061c

Move LogEntry to separate module

b4fc3aa

chfast force-pushed the refactor-extvmface branch from f55c3c1 to b4fc3aa Compare September 20, 2017 09:31

gumb0 reviewed Sep 20, 2017

View reviewed changes

gumb0 approved these changes Sep 20, 2017

View reviewed changes

chfast merged commit 851ed50 into develop Sep 20, 2017

chfast deleted the refactor-extvmface branch September 20, 2017 21:21

chfast removed the needs review label Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ExtVMFace #4496

Refactor ExtVMFace #4496

chfast commented Sep 13, 2017 •

edited

Loading

winsvega commented Sep 13, 2017

gumb0 Sep 15, 2017

gumb0 Sep 15, 2017

gumb0 Sep 15, 2017

chfast Sep 15, 2017

gumb0 Sep 15, 2017

gumb0 Sep 15, 2017

gumb0 Sep 15, 2017

chfast Sep 15, 2017

gumb0 Sep 15, 2017

gumb0 Sep 15, 2017

gumb0 Sep 15, 2017

pirapira Sep 15, 2017

chfast Sep 20, 2017

pirapira Sep 15, 2017

pirapira Sep 15, 2017

chfast Sep 19, 2017

codecov-io commented Sep 20, 2017 •

edited

Loading

chfast commented Sep 20, 2017

gumb0 Sep 20, 2017

chfast Sep 20, 2017

		#include <boost/variant/variant.hpp>
		#include <libdevcore/RLP.h>

		@@ -23,6 +23,7 @@

		#pragma once

		#include <libdevcrypto/Common.h>

Refactor ExtVMFace #4496

Refactor ExtVMFace #4496

Conversation

chfast commented Sep 13, 2017 • edited Loading

winsvega commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 20, 2017 • edited Loading

Codecov Report

chfast commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chfast commented Sep 13, 2017 •

edited

Loading

codecov-io commented Sep 20, 2017 •

edited

Loading