Issue #1032 Use null safe method for bytes to hex string conversion #1069

kishansagathiya · 2018-05-09T08:33:55Z

This is does not include all such cases
Fix for #1032

@zilm13 Since I would be using the same approach throughout all such cases, i just wanted to get this validated if this approach looks good to you.

kishansagathiya · 2018-05-09T08:35:42Z

@zilm13 If you are ok with static imports, I will use the same thing throughout all files wherever I make changes

coveralls · 2018-05-09T10:53:26Z

Coverage decreased (-0.2%) to 55.861% when pulling a5f4e74 on kishansagathiya:issue_1032 into cac6ade on ethereum:develop.

kishansagathiya · 2018-05-09T11:09:25Z

To avoid making this a huge PR, I have added changes only to the implementation and once this is merged I will make another for test, Is that cool?

Also, I have made changes only at those place where we are printing or logging some message ( i.e, logs, some println, exception or error messages etc ). Have avoided changing toString() functions. I can do that with subsequent PRs with making sure that my new change don't avoid throwing an error that should been thrown otherwise.

zilm13 · 2018-05-09T13:58:02Z

@kishansagathiya

Have avoided changing toString() functions

We need to change them too. It's object logging that fails in case of one field is null.

You'd better move file by file, so I could check that it's done for this file but as you already did it for many files, just add it to toString methods too and I will check everything together. From first view you did it ok, but it needs more attention with branch checkout before merge, so it's better to finish it for toString methods too or any other methods that are called from logging.

kishansagathiya · 2018-05-11T12:48:32Z

@zilm13 ready for review. I hope none of the toString() methods are being used for anything except logging, otherwise this might break things.

zilm13

Looks good, but few changes required according to comments.

change it here too: https://github.com/ethereum/ethereumj/blob/develop/ethereumj-core/src/main/java/org/ethereum/sync/SyncPool.java#L340
+4 lines in VM.dumpLine() used in dumpLogger, everything ending with getNoLeadZeroesData() in input

And it would be good!

zilm13 · 2018-05-15T18:45:14Z

ethereumj-core/src/main/java/org/ethereum/core/TransactionExecutor.java

@@ -45,6 +44,7 @@
 import static org.ethereum.util.ByteUtil.toHexString;
 import static org.ethereum.vm.VMUtils.saveProgramTraceFile;
 import static org.ethereum.vm.VMUtils.zipAndEncode;
+import static org.ethereum.util.ByteUtil.toHexString;


duplicate import

zilm13 · 2018-05-15T20:37:01Z

ethereumj-core/src/main/java/org/ethereum/datasource/rocksdb/RocksDbDataSource.java

@@ -40,6 +40,7 @@
 import java.util.concurrent.locks.ReentrantReadWriteLock;

 import static java.lang.System.arraycopy;
+import static org.ethereum.util.ByteUtil.toHexString;


unused org.spongycastle.util.encoders.Hex import above

zilm13 · 2018-05-15T20:43:09Z

ethereumj-core/src/main/java/org/ethereum/net/eth/message/GetBlockBodiesMessage.java

@@ -25,6 +25,8 @@
 import java.util.ArrayList;
 import java.util.List;

+import static org.ethereum.util.ByteUtil.toHexString;


unused org.spongycastle.util.encoders.Hex import above

zilm13 · 2018-05-15T20:44:18Z

ethereumj-core/src/main/java/org/ethereum/net/eth/message/NewBlockMessage.java

@@ -25,6 +25,8 @@

 import java.math.BigInteger;

+import static org.ethereum.util.ByteUtil.toHexString;


unused org.spongycastle.util.encoders.Hex import above

zilm13 · 2018-05-15T20:44:52Z

ethereumj-core/src/main/java/org/ethereum/net/eth/message/StatusMessage.java

@@ -25,6 +25,8 @@

 import java.math.BigInteger;

+import static org.ethereum.util.ByteUtil.toHexString;


unused org.spongycastle.util.encoders.Hex import above

zilm13 · 2018-05-15T20:48:43Z

ethereumj-core/src/main/java/org/ethereum/net/rlpx/PongMessage.java

@@ -101,7 +97,7 @@ public String toString() {
        long currTime = System.currentTimeMillis() / 1000;

        String out = String.format("[PongMessage] \n token: %s \n expires in %d seconds \n %s\n",
-                Hex.toHexString(token), (expires - currTime), super.toString());
+        ByteUtil.toHexString(token), (expires - currTime), super.toString());


correct spacing, please

zilm13 · 2018-05-15T20:57:36Z

ethereumj-core/src/main/java/org/ethereum/vm/program/invoke/ProgramInvokeFactoryImpl.java

-                    Hex.toHexString(lastHash.getData()),
-                    Hex.toHexString(coinbase.getLast20Bytes()),
+                    ByteUtil.toHexString(callValue.getNoLeadZeroesData()),
+                    data == null ? "" : ByteUtil.toHexString(data),


As you've changed it to null-safe, condition checking is no more required

This commit replaces use of `org.spongycastle.util.encoders.Hex.toHexString()` (Not null safe) with `org.ethereum.util.ByteUtil.toHexString()` (null safe) while logging and in all toString() methods. This covers only logging in the implementation(`/src/main`) and doesn't cover any logging in tests(`/src/tests`) Issue ethereum#1032

`hint` and `contract` are only being used in logs. Changing the toHexString function for them should not break anything. Issue ethereum#1032

Removed unused imports and other changes as asked for Issue ethereum#1032

More conversions Issue ethereum#1032

kishansagathiya · 2018-05-16T06:09:04Z

ethereumj-core/src/main/java/org/ethereum/net/swarm/Manifest.java

+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with the ethereumJ library. If not, see <http://www.gnu.org/licenses/>.
+ */


This is weird. The comment is not changed at all, but GitHub is showing it to be changed.

kishansagathiya · 2018-05-16T06:10:08Z

ethereumj-core/src/main/java/org/ethereum/samples/EventListenerSample.java

@@ -225,7 +225,7 @@ public void onPendingTransactionUpdate(TransactionReceipt txReceipt, PendingTran
    }

    public void requestFreeEther(byte[] addressBytes) {
-        String address = "0x" + Hex.toHexString(addressBytes);
+        String address = "0x" + toHexString(addressBytes);


address is used only for logging, so no harm changing it here.

kishansagathiya · 2018-05-16T06:11:43Z

ethereumj-core/src/main/java/org/ethereum/vm/program/InternalTransaction.java

-import static org.apache.commons.lang3.ArrayUtils.isEmpty;
-import static org.apache.commons.lang3.ArrayUtils.nullToEmpty;
-import static org.ethereum.util.ByteUtil.toHexString;
-


because duplicate imports

kishansagathiya · 2018-05-16T06:13:32Z

ethereumj-core/src/main/java/org/ethereum/vm/VM.java

@@ -1255,7 +1255,7 @@ else if (oldValue != null && newValue.isZero()) {
                    program.setHReturn(hReturn);

                    if (logger.isInfoEnabled())
-                        hint = "data: " + Hex.toHexString(hReturn)
+                        hint = "data: " + toHexString(hReturn)


hint is used only in logs

kishansagathiya · 2018-05-16T06:13:55Z

ethereumj-core/src/main/java/org/ethereum/vm/VM.java

@@ -1401,7 +1401,7 @@ private void dumpLine(OpCode op, long gasBefore, long gasCost, long memWords, Pr
            }

            int level = program.getCallDeep();
-            String contract = Hex.toHexString(program.getOwnerAddress().getLast20Bytes());
+            String contract = toHexString(program.getOwnerAddress().getLast20Bytes());


contract is used only in logs

zilm13 · 2018-05-16T21:09:50Z

Thank you!

kishansagathiya force-pushed the issue_1032 branch from de84dd4 to b971cf0 Compare May 9, 2018 10:28

kishansagathiya force-pushed the issue_1032 branch from b971cf0 to 1c831ee Compare May 9, 2018 10:59

kishansagathiya changed the title ~~[WIP] Issue #1032 Use null safe method for bytes to hex string conversion~~ Issue #1032 Use null safe method for bytes to hex string conversion May 9, 2018

kishansagathiya changed the title ~~Issue #1032 Use null safe method for bytes to hex string conversion~~ [WIP]Issue #1032 Use null safe method for bytes to hex string conversion May 11, 2018

kishansagathiya force-pushed the issue_1032 branch from 1c831ee to 654e0ab Compare May 11, 2018 12:26

kishansagathiya changed the title ~~[WIP]Issue #1032 Use null safe method for bytes to hex string conversion~~ Issue #1032 Use null safe method for bytes to hex string conversion May 11, 2018

zilm13 requested changes May 15, 2018

View reviewed changes

kishansagathiya added 4 commits May 16, 2018 10:53

Use null safe method to convert bytes to hexString

a8008c1

`hint` and `contract` are only being used in logs. Changing the toHexString function for them should not break anything. Issue ethereum#1032

Use null safe method to convert bytes to hexString

510ce2d

Removed unused imports and other changes as asked for Issue ethereum#1032

Use null safe method to convert bytes to hexString

a5f4e74

More conversions Issue ethereum#1032

kishansagathiya force-pushed the issue_1032 branch from 654e0ab to a5f4e74 Compare May 16, 2018 05:24

kishansagathiya commented May 16, 2018

View reviewed changes

zilm13 merged commit e6a5dd6 into ethereum:develop May 16, 2018

zilm13 mentioned this pull request May 16, 2018

Replace Hex.toHexString in logging, toString etc to safer alternative #1032

Closed

kishansagathiya mentioned this pull request May 23, 2018

Replace Hex.toHexString in logging, toString etc to safer alternative in Tests #1081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #1032 Use null safe method for bytes to hex string conversion #1069

Issue #1032 Use null safe method for bytes to hex string conversion #1069

kishansagathiya commented May 9, 2018

kishansagathiya commented May 9, 2018

coveralls commented May 9, 2018 •

edited

Loading

kishansagathiya commented May 9, 2018

zilm13 commented May 9, 2018

kishansagathiya commented May 11, 2018 •

edited

Loading

zilm13 left a comment •

edited

Loading

zilm13 May 15, 2018

zilm13 May 15, 2018

zilm13 May 15, 2018

zilm13 May 15, 2018

zilm13 May 15, 2018

zilm13 May 15, 2018

zilm13 May 15, 2018

kishansagathiya May 16, 2018

kishansagathiya May 16, 2018

kishansagathiya May 16, 2018

kishansagathiya May 16, 2018

kishansagathiya May 16, 2018

zilm13 commented May 16, 2018

		@@ -25,6 +25,8 @@

		import java.math.BigInteger;

		import static org.ethereum.util.ByteUtil.toHexString;

Issue #1032 Use null safe method for bytes to hex string conversion #1069

Issue #1032 Use null safe method for bytes to hex string conversion #1069

Conversation

kishansagathiya commented May 9, 2018

kishansagathiya commented May 9, 2018

coveralls commented May 9, 2018 • edited Loading

kishansagathiya commented May 9, 2018

zilm13 commented May 9, 2018

kishansagathiya commented May 11, 2018 • edited Loading

zilm13 left a comment • edited Loading

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

zilm13 commented May 16, 2018

coveralls commented May 9, 2018 •

edited

Loading

kishansagathiya commented May 11, 2018 •

edited

Loading

zilm13 left a comment •

edited

Loading