Refactor code

[ghost]

• Add consistent coding style
• Create a new method for the Bloxberg client to send the received raw block encapsulated with our own datatype

[kiview]

Since @lslada and I discussed this in Slack, I want to add it here, so it does not get locked from the others.

You don't want to define your own generic type here, you just use the type that you return.

Haven't run it yet, but type should be something like this, already in the field public List<EthBlock.TransactionResult<Transaction>> transactions;

    public List<EthBlock.TransactionResult<Transaction>> getTransactions() {
        return transactions;
    }

(I am not sure if the generic type inside TransactionResult is response.Transaction, you need to investigate this, I just aligned with the return type of ethBlock.getTransactions(); as documented.

On another note, maybe just work with List<EthBlock.TransactionResult> until you know the enclosed type.

[ghost]

Some addresses have the value null, which can already be seen in the current json file. In those cases, trying to get their code (for analyzing if they belong to a SC) gives a NullPointerException, even though I add an if-else statement. Any ideas on how I can solve this?

public String getCode() throws IOException {
        String returnCode;
        if (this.address == null){
            returnCode ="nullAddress";

        }  else {
            EthGetCode code = web3j.ethGetCode(this.address, DefaultBlockParameter.valueOf("latest")).send();
             returnCode = code.getCode();
        }
        return returnCode;
    }

[ghost]

Some addresses have the value null, which can already be seen in the current json file. In those cases, trying to get their code (for analyzing if they belong to a SC) gives a NullPointerException, even though I add an if-else statement. Any ideas on how I can solve this?

public String getCode() throws IOException {
        String returnCode;
        if (this.address == null){
            returnCode ="nullAddress";

        }  else {
            EthGetCode code = web3j.ethGetCode(this.address, DefaultBlockParameter.valueOf("latest")).send();
             returnCode = code.getCode();
        }
        return returnCode;
    }

While debugging, I realized that, even though there are transactions that get the null value, the problem is that I need to use the web3j object outside the Bloxberg client. and that is what I get as null.

[kiview]

Where exactly does the NPE appear, which line in the code? Also instead of the if-else statement, you can use Optional.

Not sure about the exact code, but looks something like this:

Optional.of(this.address)
  .map(String a -> 
      web3j.ethGetCode(this.a, DefaultBlockParameter.valueOf("latest")).send().getCode()
  }).orElse("")

[ghost]

The NPE appears in line 7 of the code I copy/pasted. The if-else statement was not the final version, it was the first one for testing purposes (make it work, make it right, make it fast). Adding a breakpoint in line 7 gives me: this.address= 0xab59a1ea1ac9af9f77518b9b4ad80942ade35088, web3j=null, which makes sense because web3j is never initialized, and is part of the Bloxberg client. The only trashy solution I can see is returning the web3j initialized object from the client, but it is something we DON'T want. Here is the full WIP of the Transaction class. I repeat that is a WIP version.

package de.internetsicherheit.brl.bloxberg.cache.ethereum;

import org.web3j.protocol.Web3j;
import org.web3j.protocol.core.DefaultBlockParameter;
import org.web3j.protocol.core.methods.response.EthGetCode;

import java.io.IOException;

public class TransactionAddress {
    private String address;
    private Web3j web3j;

    public TransactionAddress(String address) {
        this.address = address;
    }

    public String getAddress() {
        return address;
    }

    public String getCode() throws IOException {
        String returnCode;
        if (this.address != null) {
            EthGetCode code = web3j.ethGetCode(this.address, DefaultBlockParameter.valueOf("latest")).send();
            returnCode = code.getCode();
        } else {
            returnCode = "nullAddress";
        }
        return returnCode;
    }
}

[kiview]

How do you expect this code to work, if you have web3j as an internal field and it is never assigned or created in the constructor or anywhere else?

NPE happens not because the address is null, it is because the web3j object is null.

Again this implementation has the mistake of having a side-effect and external interaction in a getter method. When creating the TransactionAddress object (e.g. in the BloxbergClient class), the fact if it has code (or even which type of address is it!) has to be determined by the assembling class that has access and knowledge about web3j.

[kiview]

Btw, although Java doesn't (currently) distinguish between plain data classes and real objects (both are implemented using class), you can still reflect the difference in the code and this is what I try to hint at in the previous comment.

TransactionAddress should be a plain data class without any logic or behaviour. It can get constructed using logic, either in a builder class that encapsulates the creation logic or having everything necessary to construct itself provided as constructor arguments. Still, there should never be things like calling out to the network (what web3j would do) inside a data class.

Also if it is a data class, using public fields instead of methods is, in general, okay, makes the code less verbose.

On a side note, with project Valhalla, Java will get real data classes, called records. Funnily enough, I wrote a blog post about it at Bealdung 😄 .

[ghost]

Yes, I mentioned that the problem was web3j variable (See the part where I mention I realized it after putting a breakpoint). I don't understand the part of the getter method. So, the only class that has direct access to web3j object is Bloxberg client. Should the code to know which type of Transaction be there?

[kiview]

Yes, see my previous comment. Either their or another builder like class.

[ghost]

I don't know why I need to solve conflicts of src/main/java/de/internetsicherheit/brl/bloxberg/cache/ethereum/ReducedTransObject.java src/main/java/de/internetsicherheit/brl/bloxberg/cache/gui/BlockDataExtractor.java. The commit I am trying to PR is e1b8afbd1928ef422bfa6745dd7aac3d03e460bd (HEAD -> miner-user-sc-identification, origin/miner-user-sc-identification)

[kiview]

Because you should rebase against master first since #34 was merged this morning.

[ghost]

The changes in the code, like using map-collector (instead of the iterator for example), have to do with performance, cleaner code?

[kiview]

Performance should be roughly the same, but this is just modern clean Java style.

[ghost]

ok, nice to know :). So, my approach for labeling the transactions is the following. Since Bloxberg client is the one that encapsulates web3j, I would create there a new method to return the block with the transactions labelled (named getBlockWithTransactionsLabelled(block)), that will be used in writeOutTransactions() in the BlockDataAggregator. getBlockWithTransactionsLabelled will use getBlockWithData method inside. getBlockWithData, on the other hand, will have the transformed transactions of type BlockTransaction, each address of BlockTransaction will be of type TransactionAddress. TransactionAddress wil have two attributes (address and type), and the method to label type as SC or Other. I hope that what I wrote is clear without having coded it yet, and I would appreciate feedback :)

[kiview]

Don't make the API from BloxbergClient so complicated. Aim to give BloxbergClient maybe 2 public methods:

• long getCurrentBlockNumber()
• bloxberg.cache.ethereum.Block getBlock(long blockNumber) (BlockWithData can be renamed to Block)

Labelling should always happen when fetching a block.

[ghost]

ok, so those two methods will be the ones exposed, then the other just private to, at list, IMHO don't have a bunch of code inside a method. Do you agree with the rest of the logic?

[kiview]

You can delete the existing public methods, no need to keep them if they are not used. Also, thinking about it, it would probably we a good idea if we also have a method Collection<Block> getBlocks(long start, end) or something like this, to potentially load batches.

TransactionAddress should not have a method to label it, it should just have an enum field type. This will be set on construction.

[ghost]

Yes, since they are used, I was just going to change the methods to private to use them internally in bloxberg.cache.ethereum.Block getBlock(long blockNumber) to have a cleaner code inside it. If the labelling does not happen in TransactionAddress, then, should it be made in transformTransactions() inside the class now called Block?

[kiview]

Either there or in BloxbergClient. Just get coding, so we have concrete discussion points.