BoxOperations refactoring, add way to ignore boxes spent in mempool for box collection #137
Conversation
…source exchangeable
…sToSpend, feeAmount into fields to reduce method overloads
…pentBoxesLoader, add ExplorerApiWithCheckerLoader
|
Additional to the tests on this repo, tested by using in ergo-wallet-app and making multiple transactions on the same block https://testnet.ergoplatform.com/en/addresses/3Wwxnaem5ojTfp91qfLw3Y4Sr7ZWVcLPvYSzTsZ4LKGcoxujbxd3 |
| @@ -131,7 +131,7 @@ object DhtUtils { | |||
| .registers(ErgoValue.of(g_y), ErgoValue.of(g_xy)) | |||
| .build | |||
|
|
|||
| val boxesToPayFee = loadTop(ctx, sender.getAddress, MinFee, new util.ArrayList[ErgoToken]()) | |||
| val boxesToPayFee = new BoxOperations(sender.getAddress).loadTop(ctx) | |||
There was a problem hiding this comment.
I like the idea of using fluent API (or builder like) design. Suggest to also introduce factory methods like BoxOperations.create(...).
There was a problem hiding this comment.
We can do that, but it does not really make any change in my opinion: Will reduce constructors to a single one, but the create method is overloaded.
There was a problem hiding this comment.
Yeah, but it is more idiomatic for builders implementation.
|
|
||
| val senders = Arrays.asList(storage.getAddressFor(NetworkType.MAINNET)) | ||
| val unsigned = new BoxOperations(senders).withAmountToSpend(amountToSend) | ||
| .withInputBoxesLoader(new ExplorerAndPoolUnspentBoxesLoader(ctx.asInstanceOf[BlockchainContextImpl])) |
There was a problem hiding this comment.
Hm, this is not good ctx.asInstanceOf[BlockchainContextImpl]. Passing ctx along should be enough for ExplorerAndPoolUnspentBoxesLoader.
There was a problem hiding this comment.
That would mean that one of the following options must be done
- getRetroFit() added to BlockchainContext() interface - we could also add
getApiClient()to get rid of the cast inOutBoxBuilderImpl - add the needed ErgoNodeFace call to
BlockchainContext()interface
I would prefer option 1, I think it is fortunate for client code to have a way to make own calls on ApiClient and Retrofit.
There was a problem hiding this comment.
One problem with ErgoNodeFacade is that it exposes (or I would say leaks) Retrofit in the signatures. Ideally all interactions with API should be done via abstracted interfaces, where Retrofit (as an implementation detail) is not exposed.
There was a problem hiding this comment.
After thinking some more about it, I would leave it as it is.
- I agree that exposing the Retrofit stuff on the interface is not a good idea.
- On the other hand, adding the needed methods on the interface makes it more cluttered and after all, other implementations of the interface will only throw an
UnsupportedOperationExceptionfor these methods - I think it is okay to have the dependency on BlockchainContextImpl as the
ExplorerAndPoolUnspentBoxesLoaderis an optional component sitting in the impl package. It does not make sense to use it for other types of contexts, or another implementation should be provided.
lib-impl/src/main/java/org/ergoplatform/appkit/impl/ExplorerAndPoolUnspentBoxesLoader.java
Outdated
Show resolved
Hide resolved
|
|
||
| /** | ||
| * A collection of utility operations implemented in terms of abstract Appkit interfaces. | ||
| * A collection of utility operations for selecting boxes and building transactions |
There was a problem hiding this comment.
This was the key point actually. By restricting the implementation to be on top of interfaces only, decouples this methods' logic from the implementation of the core Appkit interfaces.
A property worth preserving.
There was a problem hiding this comment.
To be honest I didn't really get the idea of the file before and so maybe I did something wrong? If you refer to the cast to BlockchaincontextImpl, that's why I extracted that implementation into an own class in the impl module. If we make the additions to the interface, can be moved to BoxOperations as an inner class.
There was a problem hiding this comment.
The idea is to clearly decouple Impl classes (or hide them) from the code, which uses abstract interfaces. Basically any code that uses interfaces alone is abstracted from the concrete detail of how those interfaces are implemented in Impl classes.
Current implementation of BoxOperations is good enough, as it doesn't use Impl classes and is written on top of the interfaces.
But I suggest to leave the phrase implemented in terms of abstract Appkit interfaces. in place.
…iV1AddressesP1Transactions
…hained tx outputs
|
For the time being, we leave it as it is. When #118 is resolved, we will add an abstract api client layer that can be implemented by the implementation layer. ExplorerAndPoolUnspentBoxesLoader will then be able to use this abstract layer. |
BoxOperations get more and more complicated because the methods were static and overloaded to be used with prover, address or list of addresses and list of tokens or no list of tokens. To simplify this, I converted the methods to non-static with object fields holding the properties that can be set and have useful defaults when not set.
Client example for this change
getCoveringBoxes method's main logic was moved from BlockChainContextImpl to BoxOperations because it makes more sense to have it there. The method is still available to call from BlockChainContextImpl, but marked Deprecated to make clients call it from BoxOperations.
BoxOperations.createProver methods are still static, but deprecated. These methods don't really fit into the class and are simple helper methods that doesn't need to be provided, so could be removed in the future.
This new structure makes it both simpler to call from clients (see changed tests) and simpler to enhance with more (optional) fields. This is used here to introduce a new field for the source to get the list of unspent boxes from. With this field, we can now override the default implementation that fetches the list of unspent boxes by address from Explorer API.
This is done with
ExplorerAndPoolUnspentBoxesLoader, an implementation that omits boxes that are already spent on mempool. This enables us to get a solution for ergoplatform/ergo-wallet-app#5 - moreover, there can be other implementations done for chained transaction support or completely different box selections.