KomodoPlatform / komodo-defi-framework

This is the official Komodo DeFi Framework repository
https://komodoplatform.com/en/docs/komodo-defi-framework/
104 stars 94 forks source link

USERHOME in confpath not working #346

Closed cipig closed 5 years ago

cipig commented 5 years ago
  {
    "coin": "SYS",
    "name": "syscoin",
    "fname": "Syscoin",
    "confpath": "USERHOME/.syscoincore/syscoin.conf",
    "rpcport": 8370,
    "pubtype": 63,
    "p2shtype": 5,
    "wiftype": 128,
    "txfee": 10000
  },

enabling a coin like SYS with curl --url "http://127.0.0.1:7783" --data "{\"userpass\":\"$userpass\",\"method\":\"enable\",\"coin\":\"SYS\",\"mm2\":1}" does not work.

Console output is cant open.(USERHOME/.syscoincore/syscoin.conf)

USERHOME should be replaced in the code with the content of userhome option from the startparameters: mm2 "{\"netid\":9999,\"gui\":\"nogui\",\"client\":1, \"userhome\":\"/${HOME#"/"}\", \"passphrase\":\"$passphrase\"}"

ArtemGr commented 5 years ago

How about ~/.syscoincore/syscoin.conf?

cipig commented 5 years ago

that would only work in Linux, but coins file is used by other OSes too, so mm2 should replace USERHOME with the path to the default user home dir of that user under the running OS as a workaround i am now replacing that manually in coins file perl -i -pe 's/USERHOME/\/home\/barterdex/' coins but i don't think Windows users will do that prior to running mm2

ArtemGr commented 5 years ago

that would only work in Linux

Good to know! And that is my point as well. Tilde is a known home denominator, it will work in any UNIX-derived shell (on Linux, macOS, Android and any number of shell derivatives), even in Windows PowerShell.

It will not work by default in Rust (cf. println! ("{}; {}", Path::new ("~") .is_dir(), Path::new ("~") .exists())) regardless of the OS, but we can implement it there. And the semantics of a tilde in the beginning of the path are kind of safer and better defined than substituting USERHOME in an arbitrary portion of the path. For example: "/bad/USERHOME/path/prepare/for/surprise" - we expect USERHOME to be substituted anywhere, and that means it can clash with existing paths. "/good/~/path/no/problem" - we expect tilde to only be substituted in the beginning of the path, which makes using it in other portions of the path perfectly safe.

What is more used in the wild, ~/path or USERHOME/path? What is more intuitive to use?

cipig commented 5 years ago

For me it is ~, but i am a crazy linux fundamentalist :-)

cipig commented 5 years ago

USERHOME is also mentioned here https://github.com/jl777/coins#bitcoin-protocol-specific-json

ArtemGr commented 5 years ago

Ah, didn't know about this, and a useful piece of documentation to have.

ArtemGr commented 5 years ago

We have two parameters here, name and confpath, which are used together in an effort to find the coin configuration. Regarding the handling of the name parameter I've looked at the MM1 code again and I don't like the idea of porting it as is into MM2. From prior experience I know that parts of that code might be broken (I've tried to use it in unit tests but then had to replace it with komodo_conf_path) and I'd like a version where we know what we're doing and have it in writing (documented in the code).

So I'm going to make a simplified and cut version of this path picking algorithm and then reimplement I-don't-know-which things when and if we need them.

Alternatively I can go ahead and implement the name behavior documented at https://github.com/jl777/coins#bitcoin-protocol-specific-json.

There's also an option of reusing the komodo_conf_path, but I'm not sure if that would make sense for all the coins out there.

TL;DR: Here's the patch, please tell me if it breaks things:

cipig commented 5 years ago

The "name"-logic was already working, else it wouldn't find any configs. Most of the coins don't need confpath, they were so clever to simply use the same logic as BTC, which means the path to the config file is ~/.bitcoin/bitcoin.conf and so the path in mm2 is ~/$name/$name.conf. Unfortunately there are also not so clever guys, who use strange and unnecessary stuff like ~/.dashcore/dash.conf.

cipig commented 5 years ago

Merged this to my branch and tested. Now it says: cant open.(bitcoin.conf) the BTC config looks like this

  {
    "coin": "BTC",
    "name": "bitcoin",
    "fname": "Bitcoin",
    "rpcport": 8332,
    "pubtype": 0,
    "p2shtype": 5,
    "wiftype": 128,
    "txfee": 20000,
    "mm2": 1
  },
cipig commented 5 years ago

if i insert "confpath": "USERHOME/.bitcoin/bitcoin.conf", to config of BTC it works. We can do that too, i mean always explicitely set confpath, but then we need to edit https://github.com/cipig/coins/blob/master/coins and insert it to 300 coins. Komodo ACs don't work at all atm: cant open.(-no-name-.conf) when enabling an AC like this

  {
    "coin": "BET",
    "asset": "BET",
    "fname": "BET",
    "rpcport": 14250
  },

I think we should keep the old ~/$name/$name.conf logic and only do the confpath stuff when confpath is set.

EDIT: when adding "confpath": "USERHOME/.komodo/BET/BET.conf" to BET, it works too

ArtemGr commented 5 years ago

@cipig , makes sense, I'll add the name logic next, but I don't readily see how it'll help with

  {
    "coin": "BET",
    "asset": "BET",
    "fname": "BET",
    "rpcport": 14250
  },

since there is no name there?

ArtemGr commented 5 years ago

We've talked further on this at Discord. On Linux the ~/$asset/$asset.conf and ~/.$name/$name.conf paths should do good.

macOS is a gray sheep, on some installations the options 1) "~/.$name/$name.conf", 2) "~/Library/Application Support/.$name/$name.conf" and 3) "$APPLICATION_SUPPORT*/.$name/$name.conf"

would lend us three different paths and we don't know which of them a particular coin implementation would use. Let's get back to this when there is a chance to test the coins on mac.

* $APPLICATION_SUPPORT here means

NSFileManager* sharedFM = [NSFileManager defaultManager];
NSArray<NSURL*>* urls = [sharedFM URLsForDirectory:NSApplicationSupportDirectory inDomains:NSUserDomainMask];