Redesign asset listing and validation infrastructure #32

blabno · 2018-03-23T11:40:42Z

No description provided.

blabno · 2018-03-23T11:42:12Z

src/main/java/bisq/core/locale/CurrencyUtil.java

@@ -95,13 +87,17 @@ public static void setBaseCurrencyCode(String baseCurrencyCode) {
    public static List<CryptoCurrency> createAllSortedCryptoCurrenciesList() {
        final List<CryptoCurrency> result = new ArrayList<>();

+        final ServiceLoader<SpecificAltCoinAddressValidator> loader = ServiceLoader.load(SpecificAltCoinAddressValidator.class);


Loader is used twice, here and in AltCoinAddressValidator. I'm not sure if it makes sense to introduce separate class like AlitCoinAddressValidators to encapsulate this one line.

blabno · 2018-03-23T11:44:39Z

src/main/java/bisq/core/locale/CurrencyUtil.java

@@ -95,13 +87,17 @@ public static void setBaseCurrencyCode(String baseCurrencyCode) {
    public static List<CryptoCurrency> createAllSortedCryptoCurrenciesList() {
        final List<CryptoCurrency> result = new ArrayList<>();

+        final ServiceLoader<SpecificAltCoinAddressValidator> loader = ServiceLoader.load(SpecificAltCoinAddressValidator.class);
+        for (SpecificAltCoinAddressValidator validator : loader) {
+            result.add(new CryptoCurrency(validator.getCurrencyCode(), validator.getCurrencyName(), validator.isAsset()));


My tase tells me that validator should not hold general info about currency, but if we'd like to introduce an additional class (i.e. BchRegistrant) with altcoin description that would force users to create an additional class. What's your take on this?

blabno · 2018-03-23T11:46:34Z

src/main/java/bisq/core/payment/validation/altcoins/bch/BchAddressValidator.java

+import org.bitcoinj.core.Address;
+import org.bitcoinj.core.AddressFormatException;
+
+public class BchAddressValidator extends AbstractSpecificAltCoinAddressValidator {


I'm not sure if we should use currency name (BitcoinCashAddressValidator) or a symbol, and if symbol then all letters in upper case or like it's now?

Use the ticker symbol, but do standard capitalization, not all caps.

cbeams · 2018-03-23T11:48:13Z

src/main/java/bisq/core/locale/CurrencyUtil.java

 import java.util.stream.Collectors;

-import lombok.extern.slf4j.Slf4j;
-


You're using an old version of IDEA that doesn't respect the new codeStyles/Project.xml file that's checked into this repository, which means you're mis-organizing imports.

Hm, it respected code style from exchange. Is this one different?

blabno · 2018-03-23T11:48:19Z

src/main/java/bisq/core/payment/validation/SpecificAltCoinAddressValidator.java

+
+import bisq.core.util.validation.InputValidator;
+
+public interface SpecificAltCoinAddressValidator {


This name is ugly. I'd rather have it called AltCoinAddressValidator but it's already taken by that class that aggregates all alts.

cbeams · 2018-03-23T11:50:38Z

src/main/java/bisq/core/payment/validation/AltCoinAddressValidator.java

+        final ServiceLoader<SpecificAltCoinAddressValidator> loader = ServiceLoader.load(SpecificAltCoinAddressValidator.class);
+        for (SpecificAltCoinAddressValidator validator : loader) {
+            validators.put(validator.getCurrencyCode(), validator);
+        }


A List<SpecificAltCoinAddressValidator> should be constructor-injected here.

(as opposed to doing the ServiceLoader lookup directly within the ctor)

You mean that there should be something else registered with Guice that would provide a list of validators?

No, I just mean normal injection, i.e. classes should take their dependencies through constructors, not create them themselves. How those deps get wired up is a different matter. Guice might have a role to play there, maybe not. I'd have to look / think about it.

Should we call it AssetProviderRegistry?

cbeams · 2018-03-23T11:51:58Z

src/main/java/bisq/core/payment/validation/AltCoinAddressValidator.java

@@ -93,6 +70,10 @@ public ValidationResult validate(String input) {
            ValidationResult regexTestFailed = new ValidationResult(false,
                    Res.get("validation.altcoin.wrongStructure", currencyCode));

+            final SpecificAltCoinAddressValidator validator = validators.get(currencyCode);


Please don't declare local variables final unless absolutely necessary.

cbeams · 2018-03-23T11:54:58Z

...ain/resources/META-INF/services/bisq.core.payment.validation.SpecificAltCoinAddressValidator

@@ -0,0 +1,2 @@
+bisq.core.payment.validation.altcoins.bch.BchAddressValidator
+bisq.core.payment.validation.altcoins.ella.EllaAddressValidator


What I had in mind here for the SPI is something that would be called, e.g. a TokenListingProvider, and implementations of that SPI would provide everything necessary to list a token, including any custom validators like this. So think self-contained, higher-level.

As I mentioned in Slack just after the comment above, perhaps what we want for naming is just TokenProvider.

And actually, we may want to take this opportunity to raise the level of abstraction one degree and start referring to these things in general as "assets", as-in "crypto assets". That term has emerged pretty strongly as the most general class that includes everything from BTC itself to altcoins (e.g. ETH, LTC) to tokens, e.g. all things ERC-20 on top of ETH.

We already distinguish within Bisq between altcoins (coins with their own chain) and "tokens" (coins that ride on another chain, e.g. ERC-20). Asset is the missing generalization here.

So I could see us calling the SPI here AssetProvider, and updating our documentation to talk about "listing an asset" on Bisq, instead of "listing a token".

So the AssetProvider would be one-stop shopping for all your Bisq asset listing needs: it would define your asset name, asset ticker symbol, asset address validation, etc.

We use isAsset in the CryptoCurrency to distinguish between pure altcoins (own blockchain) and assets like ERC-20. So far it is only used to show an extra popup.

cbeams

NACK, per single comments already added. Thanks for getting this up for early review, @blabno.

Feel free to add more commits, and if you want to --amend / interactively rebase the commits you already have here, that's fine, please do (I'd ask you to clean them up into atomic commits prior to merging in any case).

blabno · 2018-03-23T14:15:05Z

src/main/java/bisq/core/payment/PaymentModule.java

+
+    @Override
+    protected final void configure() {
+        bind(AssetProviderRegistry.class).toInstance(AssetProviderRegistry.getInstance());


This will require a change in bisq-desktop to include PaymentModule or it will hit runtime exception.

cbeams · 2018-03-26T08:31:53Z

I just rebased this against master (93baf56) and force pushed. Will follow up with a few review commits now.

This change builds on initial work in prior commits to fundamentally redesign the way we manage assets in Bisq. Previously, assets were "listed" by adding them in several places, mainly AltCoinAddressValidator and CurrencyUtil. This led to large and growing classes that do everything and that are prone to merge conflicts any time there is more than one open pull request to list an asset. Now we have a new, top-level 'bisq.asset' package (we may want to extract this to its own repository for several reasons). Within, we have a first class concept of an 'Asset' (think "crypto asset", as in the most general term possible for all different kinds of crypto currencies, tokens, colored coins, etc.). Assets implementations are listed in the META-INF/services/bisq.asset.Asset service-provider file. Two subtypes of Asset exist: Coin and Token. 'Coin' is what we have always meant when saying "altcoin" so far: i.e. a coin that has its own blockchain. A 'Token' is an asset that does not have its own blockchain, but rides on top of another through a smart contract, e.g. ERC-20 tokens. And indeed, there is an abstract Erc20Token subtype that captures this common suitation and provides various conveniences. In this commit, several assets have been extracted, including Bitcoin, Bitcoin Cash, Ether, Instacash and Ellaism. Each represents an at least somewhat different kind of asset, and therefore provided a good basis for ensuring that the changes in this commit were sufficiently generalized to accommodate the other asset extractions that will follow in subsequent commits: - Bitcoin is a base currency for Bisq - Bitcoin Cash is a typcial Bitcoin-based altcoin - Instacash is another Bitcoin-based altcoin - Ether is a non Bitcoin-based altcoin - Ellaism is an ERC-20 token A number of changes remain to be made in the new types introduced here, including Javadoc, and this will be added prior to merging the series of commits that will make up the overall pull request. Also note that the tests in AltCoinAddressValidatorTest have been left intact for now, as a means of double-checking that everything works as expected from the application level. It should also be noted that AltCoinAddressValidator is essentially becoming a translation layer (an "anti-corruption layer" in DDD terms) where the new 'bisq.asset' world meets the old AltCoinAddressValidator / CurrencyUtil world that bisq-desktop already knows. It is intentional that this refactoring has been isolated in this way, leaving the old "contract" for AltCoinAddressValidator and CurrencyUtil intact. We can see about refactoring bisq-desktop to deal more directly with the new AssetRegistry interface in the future, but it is not necessary, and indeed not desirable to do all of that now in this PR. The idea is to redesign the underlying infrastructure now, enough to make it more manageable to deal with going forward.

Note that the existing OctocoinAddressValidator was nothing more than a copied and pasted Base58 bitcoin address validator (identical to PNCAddressValidator, incidentally, which will also be deleted when we extract PNC).

cbeams · 2018-03-27T13:19:53Z

@blabno, please study the commits I've added above, and be sure to read the full comment in cab46e2. Could you follow suit with what I've started here by continuing to extract each remaining asset, one commit at a time? You can push commits up as soon as you have them and I'll review them as quickly as possible to make sure everything is on track.

Of course if you have questions or concerns, please bring those up too, here or in Slack. Thanks!

cbeams

ACK.

@blabno, please have a look through the final review commits I pushed above.

Note that I'm mid-flight on updating the https://bisq.network/list-token document to reflect these changes, but I wanted to get this PR merged asap, in case, @blabno, you wanted to include this work in this month's compensation request. There is no rush to do so, by the way, and you can feel free to defer this until next month's, when the actual value of these changes has had some time to become clear. Either way is fine.

Redesign asset listing and validation infrastructure

And preserve old /list-token links by redirecting to new /list-asset. See bisq-network/bisq-core#32

See bisq-network/bisq-core#32

cbeams · 2018-04-05T11:24:58Z

@ManfredKarrer, @blabno, see my commits above to docs and website. The old https://bisq.network/list-token link is now https://bisq.network/list-asset, and the doc has been fully reworked to reflect the new structure and process (bisq-network/bisq-docs@c0e9ffc).

I will now let authors of all outstanding asset listing pull requests know that they need to rework their PRs to adhere to the new instructions.

blabno · 2018-04-05T11:26:48Z

I can do the rework for those PRs.

blabno · 2018-04-05T11:36:27Z

Entries in this file must be kept in alphabetical order.

Probably we should add that the sorting is case insensitive

In section Write tests for your asset I would add that the test should go into the same package as the new asset.

We should probably mention that there should be no additional classes. If any is required then make it an inner class of new asset.

cbeams · 2018-04-05T11:39:20Z

Probably we should add that the sorting is case insensitive

Yeah, it's documented in the file itself. Didn't want to duplicate that too much.

In section Write tests for your asset I would add that the test should go into the same package as the new asset.

Probably a good idea.

We should probably mention that there should be no additional classes. If any is required then make it an inner class of new asset.

Probably a good idea, but should also be covered by the "follow existing conventions" tip.

Feel free to patch the doc to add these items if you like. Otherwise, I'd say let's see how actual PRs play out and tweak / tune the doc as necessary along the way.

Thanks for the review though.

cbeams · 2018-04-05T11:40:04Z

I can do the rework for those PRs.

We decided in Slack to close all the existing PRs and have authors resubmit, as a way to get early validation that the new instructions are sufficient.

Individual asset address validation tests are now handled in dedicated unit tests. This test is now essentially an integration test, ensuring that AltCoinAddressValidator interacts correctly with the underlying AssetRegistry infrastructure. This work was intended to be part of bisq-network#32.

@blabno

This appears to be dead code, see @blabno's inquiry about it at https://bisq.slack.com/archives/C1267HFD1/p1521801524000052. If this is in fact *not* dead code, i.e. it gets used to print out certain reports at release boundaries, I suggest we still kill it here, and re-write that code in such a way that it (a) uses the new bisq.asset infrastructure and (b) doesn't require duplicating ticker symbols in this class. This work was intended for inclusion in bisq-network#32.

blabno commented Mar 23, 2018

View reviewed changes

cbeams reviewed Mar 23, 2018

View reviewed changes

blabno commented Mar 23, 2018

View reviewed changes

cbeams reviewed Mar 23, 2018

View reviewed changes

cbeams suggested changes Mar 23, 2018

View reviewed changes

blabno commented Mar 23, 2018

View reviewed changes

Bernard Labno added 4 commits March 26, 2018 10:20

Move altcoin address validators to separate classes.

ba4ac0f

AssetProvider

856669a

AssetProviderRegistry

cb6c579

Adjust Bch and Ella asset providers names.

c949a24

cbeams force-pushed the feature/alt-validator-refactoring branch from 8ba96b0 to c949a24 Compare March 26, 2018 08:31

cbeams added 11 commits March 26, 2018 10:32

Organize imports

695053d

Extract BSQ

9026200

Extract Litecoin

9fbed62

Extract Dogecoin

c2c7e40

Extract Dash

bd37996

Remove unused imports

f07e13e

Extract PIVX

a08ac11

Extract Internet of People

22cd1b4

Add missing license headers

df0931e

Extract Octocoin

981ca38

Note that the existing OctocoinAddressValidator was nothing more than a copied and pasted Base58 bitcoin address validator (identical to PNCAddressValidator, incidentally, which will also be deleted when we extract PNC).

cbeams assigned blabno Mar 27, 2018

cbeams mentioned this pull request Mar 27, 2018

Add 'bisq.asset' to IDEA import layout table bisq-network/bisq-multirepo#1

Merged

cbeams added 2 commits April 3, 2018 13:49

Add Javadoc for bisq.asset types

990070f

Document META-INF/services/bisq.asset.Asset

2f231d3

cbeams force-pushed the feature/alt-validator-refactoring branch from 3277cfd to 2f231d3 Compare April 3, 2018 11:49

cbeams approved these changes Apr 3, 2018

View reviewed changes

cbeams merged commit 2f231d3 into bisq-network:master Apr 3, 2018

cbeams added a commit that referenced this pull request Apr 3, 2018

Merge pull request #32 from blabno/feature/alt-validator-refactoring

c58ad46

Redesign asset listing and validation infrastructure

blabno deleted the feature/alt-validator-refactoring branch April 3, 2018 12:08

cbeams added a commit to bisq-network/bisq-website that referenced this pull request Apr 5, 2018

Add /list-asset redirect to point to new process doc

b814ffa

And preserve old /list-token links by redirecting to new /list-asset. See bisq-network/bisq-core#32

cbeams added a commit to bisq-network/bisq-docs that referenced this pull request Apr 5, 2018

Rename list-token.adoc => list-asset.adoc

b491fed

See bisq-network/bisq-core#32

cbeams added a commit to bisq-network/bisq-docs that referenced this pull request Apr 5, 2018

Update list-asset.adoc to reflect new process

c0e9ffc

See bisq-network/bisq-core#32

cbeams mentioned this pull request Apr 5, 2018

Rework AltCoinAddressValidatorTest into integration test #53

Merged

blabno mentioned this pull request Apr 30, 2018

For January - April 2018 bisq-network/compensation#61

Closed

cbeams mentioned this pull request May 3, 2018

For April 2018 bisq-network/compensation#68

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign asset listing and validation infrastructure #32

Redesign asset listing and validation infrastructure #32

blabno commented Mar 23, 2018

blabno Mar 23, 2018

blabno Mar 23, 2018

blabno Mar 23, 2018

cbeams Mar 23, 2018

cbeams Mar 23, 2018

blabno Mar 23, 2018

blabno Mar 23, 2018

cbeams Mar 23, 2018

cbeams Mar 23, 2018

blabno Mar 23, 2018

cbeams Mar 23, 2018

blabno Mar 23, 2018

cbeams Mar 23, 2018

cbeams Mar 23, 2018

cbeams Mar 23, 2018

ManfredKarrer Mar 23, 2018

cbeams left a comment

blabno Mar 23, 2018

cbeams commented Mar 26, 2018

cbeams commented Mar 27, 2018

cbeams left a comment

cbeams commented Apr 5, 2018

blabno commented Apr 5, 2018

blabno commented Apr 5, 2018 •

edited

Loading

cbeams commented Apr 5, 2018

cbeams commented Apr 5, 2018

		import java.util.stream.Collectors;

		import lombok.extern.slf4j.Slf4j;


		import bisq.core.util.validation.InputValidator;

		public interface SpecificAltCoinAddressValidator {

		@@ -0,0 +1,2 @@
		bisq.core.payment.validation.altcoins.bch.BchAddressValidator
		bisq.core.payment.validation.altcoins.ella.EllaAddressValidator

Redesign asset listing and validation infrastructure #32

Redesign asset listing and validation infrastructure #32

Conversation

blabno commented Mar 23, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeams commented Mar 26, 2018

cbeams commented Mar 27, 2018

cbeams left a comment

Choose a reason for hiding this comment

cbeams commented Apr 5, 2018

blabno commented Apr 5, 2018

blabno commented Apr 5, 2018 • edited Loading

cbeams commented Apr 5, 2018

cbeams commented Apr 5, 2018

blabno commented Apr 5, 2018 •

edited

Loading