Skip to content
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

Deposit contract handling using Web3J via JSON-RPC #170

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

zilm13
Copy link
Member

@zilm13 zilm13 commented Aug 14, 2019

pow:web3j implementation plus some fixes to pow:ethereumj

@zilm13 zilm13 requested a review from mkalinin August 14, 2019 21:32
Copy link
Contributor

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

Great job is done! I left a few requests for improvement.


import java.util.Scanner;

public class ContractSource {
Copy link
Contributor

Choose a reason for hiding this comment

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

A thought of making this class a singletone. This is rather a cosmetic change than something functional though.

@@ -10,7 +10,9 @@ dependencyManagement {

dependency "org.apache.logging.log4j:log4j-api:${log4j2Version}"
dependency "org.apache.logging.log4j:log4j-core:${log4j2Version}"
dependency "org.slf4j:slf4j-simple:1.7.28"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need slf4j if we already have log4j2?


RUN apk add --no-cache make gcc musl-dev linux-headers git

ARG branch=master
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to stick with some tagged version in order to prevent unnecessary code and/or dependency updates on the geth side that could break docker container or the test itself.

*
* <p>Generated with web3j version 4.3.2-SNAPSHOT.
*
* <p>Generated with following input: org.web3j.codegen.SolidityFunctionWrapperGenerator.java -b /home/work/projects/beacon-chain-java/pow/core/src/main/resources/org/ethereum/beacon/pow/ContractBin.bin -a /home/work/projects/beacon-chain-java/pow/core/src/main/resources/org/ethereum/beacon/pow/ContractAbi.json -o /home/work/projects/beacon-chain-java/pow/web3j/src/main/java -p org.ethereum.beacon.pow.contract </p>
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to reuse this script later on. Would you mind to include it into a codebase and leave a reference to it here?

web3RequestExecutor.executeOnSyncDone(
() -> {
processConfirmedBlocks();
onEthereumUpdated();
Copy link
Contributor

Choose a reason for hiding this comment

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

onEthereumUpdated() in its turn calls to processConfirmedBlocks(). Is that by design?

block -> {
onEthereumUpdated();
})
.subscribe();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean that we're subscribed to blocks from very beginning before sync is done? We might want to get this subscription right on sync done event. Is that doable with web3j? Probable obstacle that I can see so far is that sync done event is not always emitted. For example, would it be emitted if sync was done before we get subscribed?

web3j
.ethGetBlockByNumber(DefaultBlockParameter.valueOf(BigInteger.valueOf(number)), false)
.sendAsync()
.join();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking of processing blocks and transaction receipts in an asynchronous fashion. We probably can get a block flow out of web3j, then add a processor that filters out blocks with usage of bloom filter, then yet another one which requests for tx receipts of valuable blocks. I guess we can created a stream upon a set of futures that are requesting for receipts. And at the end parse deposit data from tx receipts upon their arrival.

This scheme has at least one flaw, deposit events might be processed in the wrong order but I don't think that there will be a big mess. So we could probably have a buffer that could be drained when there is a deposit standing next to last processed one.

BigInteger nonce = txCountFut.join().getTransactionCount();
BigInteger value =
Convert.toWei(amount.longValue() + "", Convert.Unit.GWEI).toBigInteger();
BigInteger gasLimit = BigInteger.valueOf(2_000_000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's create a constant for this magic number.


@Override
public CompletableFuture<BytesValue> signTransaction(
BytesValue unsignedTransaction, BytesValue eth1PrivKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to avoid private key management and dedicate it to geth or parity. Any thoughts on that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've created issue #169

```
Running container:
```shell script
docker run -d -p 8545:8545 -p 30303:30303 <image id>
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to give this image a name: docker build -t name && docker run -d name.


@Before
public void setUp() throws Exception {
this.web3j = Admin.build(new HttpService());
Copy link
Contributor

Choose a reason for hiding this comment

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

Since RPC URL may vary from one env to another it would be better if it's explicitly passed to web3j.

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.

None yet

2 participants