-
Notifications
You must be signed in to change notification settings - Fork 9
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
pow:web3j
implementation plus some fixes topow:ethereumj