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

Script engine support for Balance #983 #992

Merged
merged 16 commits into from
Aug 29, 2020

Conversation

AwooOOoo
Copy link
Contributor

Added balance support into the script engine (#983). It is set up similar to setTick in that it takes a callback and the trading pair to return the balance on.

Fields Available
The balance exposes the following fields;
[currency=ETH, total=1000, available=1000, frozen=0, borrowed=0, loaned=0, withdrawing=0, depositing=0]

Note that the response is called twice, once for the base and once for the quote. Assuming you had ETH/EUR selected, in the example below, it would call the function once for ETH and once for EUR.

Example Usage:

function start() {
  var subscription = balance.get(
    function(event) {
      var b = event.balance();  
      notifications.info("Balance: " + b.currency + " - " + b.available);
    },
    parameters.selectedCoin
  );
  
  return RUNNING
}

function stop() {
  events.clear(subscription);  // Stop the subscription
}

Output from Example:
image

Possible Improvements:

  1. Since it is already a balance.get() call, perhaps don't have the .balance() on the return
  2. Combine returns so you can access .base() & .count() at the same time to ensure they are not out of sync
  3. Make it a single call and not a subscription

Thoughts?

@AwooOOoo
Copy link
Contributor Author

AwooOOoo commented Aug 17, 2020

@badgerwithagun - I've made some mark down updates on some of the wikis, but don't have permission to push changes and can't fork to do a PR. Whats the best way to submit them?

@badgerwithagun
Copy link
Member

This looks great. Could we move the new method toEvents, though, and use the same call pattern as for tickets, e.g. setBalance, with the assumption that it will be called again on shutdown to release the subscription?

@badgerwithagun
Copy link
Member

One other thought, probably not one to solve here, is that the ticker and balance event objects are not version controlled DTOs, and we should probably create some for this. If those objects change, we can't have that propagating to the script API without careful consideration.

@AwooOOoo
Copy link
Contributor Author

AwooOOoo commented Aug 19, 2020

@badgerwithagun ,
No worries, I'll move it to Events. Assign the issue to me also. Good point on the versioning, I'll have a think about it.

Copy link
Member

@badgerwithagun badgerwithagun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Just one small cleanup then I think we're good to go.

@badgerwithagun
Copy link
Member

One more thing: could you extend the testing in TestScriptJobProcessor to cover these use cases?

@AwooOOoo
Copy link
Contributor Author

Code Cleanup submitted. Its going to take me a little bit to wrap my head around how to write those test cases well.

@AwooOOoo
Copy link
Contributor Author

Is there a shortcut to just running the TestScriptJobProcessor tests? I'm still getting familiar with the environment.

@badgerwithagun
Copy link
Member

Do you use a Java IDE, @AwooOOoo ?

This is easy in IntelliJ, just right-click and run.

@AwooOOoo
Copy link
Contributor Author

@badgerwithagun Currently using Visual Studio Code. I installed eclipse, but haven't gotten into it yet and it seems to have a steep learning curve. Sorry if the PR's aren't efficient. I haven't programmed Java in many years and so am pretty damn rusty. I appreciate the hints, its a lot to take in trying to remember the language and take in the code base.

@badgerwithagun
Copy link
Member

@badgerwithagun Currently using Visual Studio Code. I installed eclipse, but haven't gotten into it yet and it seems to have a steep learning curve. Sorry if the PR's aren't efficient. I haven't programmed Java in many years and so am pretty damn rusty. I appreciate the hints, its a lot to take in trying to remember the language and take in the code base.

Got it. FWIW, IntelliJ is by far the easiest to use with this codebase. If you load it as a Maven project it will work out of the box with no further configuration.

@AwooOOoo
Copy link
Contributor Author

This is easy in IntelliJ, just right-click and run.

It is easy with IntelliJ, thanks =)

…returns.

- Cleaned up some warnings reported by IntelliJ
@badgerwithagun badgerwithagun merged commit d2b93e9 into gruelbox:master Aug 29, 2020
@badgerwithagun
Copy link
Member

Merged. Thanks @AwooOOoo :)

@AwooOOoo
Copy link
Contributor Author

AwooOOoo commented Aug 30, 2020

@badgerwithagun There is a PR in the wiki, that covers the changes. I'll add the unit tests when I get some time.

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.

2 participants