GENERALBYTESCOM / batm_public

BATM Public Repository
www.generalbytes.com
Other
98 stars 241 forks source link

Testing question #175

Closed dwasyluk closed 5 years ago

dwasyluk commented 5 years ago

We have made some significant changes to the BATM software and want to test them on the terminal prior to issuing a PR back to the GB repo. We are running our own CAS server and all the tests are passing after our changes but they don't seem to exercise much beyond reaching out to the price oracle.

Is there a way to test the actual terminal experience through CAS prior to a PR or is the only way to test things on a terminal through an official PR to GB and then waiting for them to issue an official release w those changes?

rpanak-generalbytes commented 5 years ago

Hi, first of all, you are right. Testing on the terminal is good practice. If you run your own server, you can do that on your own. You just have to push your code to the server installation. The extension JAR (build output of project server_extensions_extra) can be found in /batm/app/master/extensions. You can replace it with your own version. You just need to be careful that the server version matches the current public API. For example, if you have server version 20190201 and your changes are based on a public repository commit on 20190212, there is a chance your extension JAR will not work if there is an incompatible change in the API.

sidhujag commented 5 years ago

We had to update the dependent bitcoin RPC library to support adding new RPC functions our coin has.. is this the right way or is it better to use web auth via API_KEYs like some other coins are doing? As I understand there are 3 dependencies to a signing node that holds funds for the ATM (getbalance, sign/send raw transaction, get new address) is there anything else?

Note: if using address based accounts in Bitcoin it is being deprecated and you can create wallets inside of Bitcoin Core to represent different accounts, I don't think the RPC library supports this yet this is potentially something that should be done before Core fully decides to remove those functions, I think getaddressbyaccount for example has issues and is going to be removed in 0.18

rpanak-generalbytes commented 5 years ago

@sidhujag What do you mean by updating the RPC library. Have you used a newer version or patched an existing one with custom code?

The thing is that this lib is used for many coins. If you introduce an incompatibility, those can stop working. The solution can be:

If you just need a newer version and it doesn't break any existing coins, that would probably be the easiest solution. Using a REST API should also be quite easy. You just use whatever REST endpoints you need in order to implement the methods in IWallet, IRateSource and IExchange (optional) interfaces (for sell you'd need additional ones).

As for the accounts/wallets: so far we have no plans to change this, but we're aware of the (possible) issue.

sidhujag commented 5 years ago

@rpanak-generalbytes Yes I created a syscoin-rpc lib and imported that package just for syscoin to do the RPC integration.. but I just was wondering what the best practice is for the maximum flexibility. Is the IExchange (Using an exchange adaptor for REST endpoints by authing via exchange API) option giving more flexibility in terms of liquidity and whatnot? I read in some comment on github it was mentioned it gives more flexibility if that is the case we will want to pursue that direction rather than creating forked rpc libraries.

rpanak-generalbytes commented 5 years ago

The starting point is always to implement some basic interfaces. IExchange is optional, but it does offer more flexibility for your coin if you implement it. It can be used for example to refill your hot wallet automatically from the exchange.

The other question is how you implement those interfaces. That's up to you, but a pattern started to emerge in our repository. People are using the bitcoin RPC client in order to implement the IWallet interface by using the bitcoin (or forked coin) daemon. Since many forks are compatible, they can still use the bitcoin RPC library for their coin. This is just a lib to talk to the coin daemon, however, so it won't help you with the exchange implementation. Or, you might want to implement the IWallet interface without the daemon, so the RPC lib won't help you.

To help with the IExchange implementation, you can use the XChange library that already implements communication with the most popular exchanges. Import the repository into a Java IDE and take a look at IExchange implementations. Some implement a simple rest API for their exchange, some use the XChange library.

Sometimes you don't need to implement the exchange because the implementation is already present. It just doesn't support the coin you're interested in. If the coin is listed on the exchange, it should be easy to enhance the code to add the support.