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

Delist untraded assets #1861

Merged
merged 3 commits into from Nov 8, 2018

Conversation

cbeams
Copy link
Member

@cbeams cbeams commented Nov 6, 2018

Per https://docs.bisq.network/exchange/howto/list-asset.html#inactive-assets-will-be-de-listed:

At each new release we will check whether already-listed assets have been traded in the past 4 months. If this requirement is not met the asset will be removed. The Bisq trade statistics are used as a reference. Removal of an un-traded asset will not be announced outside of normal release notes.

@ManfredKarrer, can you put together a PR for this for 0.9.0? In subsequent releases, perhaps @blabno can keep an eye on this and put together similar PRs that follow the template you lay out in this first one.

And can you also include a description as to how you create the list of assets to be delisted based on the trade statistics? Again, this would be so that others can easily follow suit (and possibly automate it) in the future.

@ManfredKarrer
Copy link
Member

I wanted to do that but on my prio list there is so much more so I am not sure if I will have time. Maybe @blabno find time to delist not traded coins. I postes at a proposal the stats and added code to easily print it out. Mid term plan was to have that automated. I think removing would already work but we should have some flexibility to re-enable them (by fee payment). Have it on my todo list for the missing dao features, but as said i keep it rather low prio...

@cbeams
Copy link
Member Author

cbeams commented Nov 3, 2018

@ManfredKarrer wrote:

Maybe @blabno find time to delist not traded coins.

Sure. I could possibly do it too, just want to make sure we do it correctly.

I posted at a proposal the stats

That's right. Here they are: bisq-network/proposals#41 (comment). That's a complete list (as of the date of the posting) of which coins should be delisted.

Is it as simple as just removing the Asset implementations, though? Is there anything else we would need to do to put together a proper backward compatible PR? We talked about implementing delisting under the DAO as logical instead of physical deletion, but that's for the future. For right now, i.e. 0.9.0, I just want to delist this stuff in the simplest way possible while not breaking compatibility.

[...] and added code to easily print it out

Can you provide a link to that code, and indicate how to run it if it's not obvious?

@blabno
Copy link

blabno commented Nov 3, 2018 via email

@cbeams
Copy link
Member Author

cbeams commented Nov 3, 2018

It should be enough to remove the entry from meta-inf/services.

Right; when asking if it's "enough" to just remove the implementations, I was referring more to compatibility concerns at the trade protocol level and across versions. Just don't want to do a naive removal of code / entries in META-INF. Perhaps it is just that simple, but wanted to double-check first.

@ManfredKarrer
Copy link
Member

ManfredKarrer commented Nov 3, 2018

@cbeams @blabno Ah actually removing code can break users accounts who have used that altcoin. So we need to mark them as hidden. I think I implemented that already (AssetService.class but its kind of WIP). I will have a look on that a bit later. Being busy atm with bonds...

I think we just can enable the automatic delisting and add a re-listing feature later...

@ManfredKarrer
Copy link
Member

ManfredKarrer commented Nov 3, 2018

The code for printing the inactive assets is in TradeStatisticsManager.checkTradeActivity(). The log at the end are set to debug level, if u change that to info u will get an up to date list.
There are still open questions on the parameters and model (see bisq-network/proposals#42). There is currently no code for removing them as that is part of the Dao class AssetService, but that could be easily added.
I think the params need to be set good so that we are not to radical (e.g. Namecoin might get delisted but there is for sure interest in NC - there are even cooperation attempts from the namecoin community).

 - Remove assets from META-INF/services/bisq.asset.Asset
 - Preserve asset types but mark as @deprecated
 - @ignore asset tests

Preserving the types is important from a compatibility perspective.
Users who have traded these assets in the past, however few there may
be, need to be able to classload the asset type(s) in order to avoid
errors when browsing through their trade portfolio history.
@cbeams cbeams self-assigned this Nov 6, 2018
@cbeams
Copy link
Member Author

cbeams commented Nov 6, 2018

I've implemented the necessary changes and converted this issue into a pull request. @ManfredKarrer, could you please review?

@blabno, for now we'll need to start adding newly listed assets to TradeStatisticsManager#isWarmingUp (as in 85caf02). This doesn't affect anything at runtime, but it makes sure that we can get accurate lists of which assets to delist in the future. It only strictly needs to be done at release boundaries.

@ManfredKarrer
Copy link
Member

Thanks @cbeams ! I need to test what happens if a user has used a coin in his altcoin accounts and if we remove it from code. Will update after that...

@ManfredKarrer
Copy link
Member

@cbeams I tested what happens if a asset get removed and there have been minor issues. Those are fixed with #1895. So I prefer to get that merged first thenI will merge that PR.

@cbeams
Copy link
Member Author

cbeams commented Nov 7, 2018

Perfect, thanks.

@ManfredKarrer
Copy link
Member

ManfredKarrer commented Nov 7, 2018

@cbeams Why is Koto and Obsidian not removed? Both have been added before 11. May.

EDIT: Ah I see they are removed from the registry but code is still there.

@ManfredKarrer
Copy link
Member

Here the current lists with changed parmeter for required volume to 0.001 BTC.

Assets to remove:
TradeCurrency(code=GBYTE, name=Byte)
TradeCurrency(code=XCP, name=Counterparty)
TradeCurrency(code=XZC, name=Zcoin)

Insufficiently traded assets:

Not traded assets:
Byte (GBYTE)
Counterparty (XCP)
Zcoin (XZC)

New assets (in warming up phase):
01coin (ZOC): Trade amount: 0.0302 BTC, number of trades: 14
Actinium (ACM): Trade amount: 0.00 BTC, number of trades: 0
Aquachain (AQUA): Trade amount: 0.0015 BTC, number of trades: 2
BitCloud (BTDX): Trade amount: 0.00 BTC, number of trades: 0
Bitcoin 2 (BTC2): Trade amount: 0.00 BTC, number of trades: 0
Bitcoin Core (BTCC): Trade amount: 0.00 BTC, number of trades: 0
Bitcoin Instant (BTI): Trade amount: 0.00 BTC, number of trades: 0
Blur (BLUR): Trade amount: 0.00 BTC, number of trades: 0
Chaucha (CHA): Trade amount: 0.00 BTC, number of trades: 0
Credits (CRDS): Trade amount: 0.00 BTC, number of trades: 0
Croat (CROAT): Trade amount: 0.00 BTC, number of trades: 0
CryptoTari (TARI): Trade amount: 0.00 BTC, number of trades: 0
Cryptonodes (CNMC): Trade amount: 0.00 BTC, number of trades: 0
DACash (DAC): Trade amount: 0.0013 BTC, number of trades: 1
DRIP (DRIP): Trade amount: 0.00 BTC, number of trades: 0
Dragonglass (DRGL): Trade amount: 0.00 BTC, number of trades: 0
EtherStone (ETHS): Trade amount: 0.00 BTC, number of trades: 0
FuturoCoin (FTO): Trade amount: 0.00 BTC, number of trades: 0
Graft (GRFT): Trade amount: 0.00 BTC, number of trades: 0
GreenBlockCoin (GBK): Trade amount: 0.00 BTC, number of trades: 0
Kekcoin (KEK): Trade amount: 0.00 BTC, number of trades: 0
LikeCoin (LIKE): Trade amount: 0.00 BTC, number of trades: 0
Lobstex (LOBS): Trade amount: 0.00 BTC, number of trades: 0
Loki (LOKI): Trade amount: 0.00 BTC, number of trades: 0
MaxCoin (MAX): Trade amount: 0.00 BTC, number of trades: 0
MegaCoin (MEC): Trade amount: 0.00 BTC, number of trades: 0
MicroCoin (MCC): Trade amount: 0.00 BTC, number of trades: 0
MobitGlobal (MBGL): Trade amount: 0.00 BTC, number of trades: 0
Motion (XMN): Trade amount: 0.00 BTC, number of trades: 0
Myriadcoin (XMY): Trade amount: 0.00 BTC, number of trades: 0
Nano (NANO): Trade amount: 0.00 BTC, number of trades: 0
Neos (NEOS): Trade amount: 0.00 BTC, number of trades: 0
NewPowerCoin (NPW): Trade amount: 0.00 BTC, number of trades: 0
Nimiq (NIM): Trade amount: 0.00 BTC, number of trades: 0
PRiVCY (PRIV): Trade amount: 0.00 BTC, number of trades: 0
PZDC (PZDC): Trade amount: 0.00 BTC, number of trades: 0
Pix (PIX): Trade amount: 0.00 BTC, number of trades: 0
PixelPropertyToken (PXL): Trade amount: 0.00 BTC, number of trades: 0
QMCoin (QMCoin): Trade amount: 0.00 BTC, number of trades: 0
Quantum Resistant Ledger (QRL): Trade amount: 0.00 BTC, number of trades: 0
Radium (RADS): Trade amount: 0.00 BTC, number of trades: 0
Ryo (RYO): Trade amount: 0.00 BTC, number of trades: 0
SUB1X (SUB1X): Trade amount: 0.00 BTC, number of trades: 0
Starwels (MAI): Trade amount: 0.00 BTC, number of trades: 0
Triton (TRIT): Trade amount: 0.00 BTC, number of trades: 0
TurtleCoin (TRTL): Trade amount: 0.00 BTC, number of trades: 0
Wavi (WAVI): Trade amount: 0.00 BTC, number of trades: 0
Zero (ZER): Trade amount: 0.00 BTC, number of trades: 0

Sufficiently traded assets (17):
Bitcoin Cash (BCH): Trade amount: 6.927 BTC, number of trades: 26
Bitcoin Clashic (BCHC): Trade amount: 0.01 BTC, number of trades: 1
Dash (DASH): Trade amount: 0.4684 BTC, number of trades: 5
Decred (DCR): Trade amount: 0.26 BTC, number of trades: 3
Dogecoin (DOGE): Trade amount: 0.001 BTC, number of trades: 1
Ether (ETH): Trade amount: 7.187 BTC, number of trades: 18
Ether Classic (ETC): Trade amount: 0.01 BTC, number of trades: 1
Gridcoin (GRC): Trade amount: 0.0717 BTC, number of trades: 4
Litecoin (LTC): Trade amount: 1.9526 BTC, number of trades: 15
Monero (XMR): Trade amount: 1214.442 BTC, number of trades: 1514
Namecoin (NMC): Trade amount: 0.21 BTC, number of trades: 4
PIVX (PIVX): Trade amount: 0.10 BTC, number of trades: 1
Siafund (SF): Trade amount: 3.15 BTC, number of trades: 4
Spectrecoin (XSPEC): Trade amount: 0.742 BTC, number of trades: 8
Unobtanium (UNO): Trade amount: 0.049 BTC, number of trades: 3
Zcash (ZEC): Trade amount: 1.224 BTC, number of trades: 3
ZenCash (ZEN): Trade amount: 0.889 BTC, number of trades: 9

@ManfredKarrer
Copy link
Member

I will merge it as it is now. Still wondering why there are some files left with deprecated annotation. And we should also remove the 3 remaining assets, but that can be done before release with an updated check.

@ManfredKarrer ManfredKarrer merged commit 247f383 into bisq-network:master Nov 8, 2018
@panterozo
Copy link

Hey guys! We were waiting the release with our coin (CHA) to start using Bisq. The merge was this, but we wont see our CHA in the list with AltCoins from the BisQ software downloaded from the download page. Please, could you help us with this issue? That's why we havn't made any trade.
I'm looking forward.
Regards

@FernandoBatalha
Copy link

FernandoBatalha commented May 9, 2019

@cbeams @ManfredKarrer

Would it be possible to list the Nano(NANO) again? When it was listed, much of the community did not know about Bisq.

Official project URL: https://nano.org
Official block explorer URL: https://www.nanode.co
Volume: https://coinmarketcap.com/currencies/nano/#markets

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

5 participants