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

enables the use of per-request api-key/secret-key passing while keeping backward compatibility. #105

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

fbettag
Copy link
Contributor

@fbettag fbettag commented Oct 28, 2023

I have used Application.set_env before, but this approach works a lot better.

feedback is welcome.

@fbettag
Copy link
Contributor Author

fbettag commented Oct 28, 2023

gonna rewrite this again

@fbettag fbettag closed this Oct 28, 2023
@dvcrn
Copy link
Owner

dvcrn commented Oct 29, 2023

I think you can inject those as extra optional parameters into optional_params, so they are becoming part of the keyword list opts: https://github.com/dvcrn/binance.ex/blob/master/lib/binance.ex#L74

Then inside client, check if those params exist, and if they do use them for overwriting the APplication.get_env stuff

Here's how I did it with a similar library (OpenAI):

https://github.com/dvcrn/ex_openai/blob/main/lib/ex_openai/client.ex#L117-L118

@fbettag
Copy link
Contributor Author

fbettag commented Oct 31, 2023

thanks for the suggestion. currently running trials but iex -S mix looks good

@fbettag
Copy link
Contributor Author

fbettag commented Oct 31, 2023

this now is nice and working. sorry for the fuzz

Copy link
Owner

@dvcrn dvcrn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left some comments, mostly cosmetic

Also, I think we need a test for this. If you're unsure how to do it, I'm happy to help / take over as well.

@@ -33,7 +33,7 @@ defmodule Binance.Rest.HTTPClient do
end
end

defp request_binance(url, body, method) do
defp request_binance(api_key, url, body, method) do
Copy link
Owner

Choose a reason for hiding this comment

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

The signature of this function is becoming a bit odd. API key should probably come after url, or in the end within optional args. Or even better - a pattern-matched function with different signature for apikey and no apikey

@@ -81,7 +81,7 @@ defmodule Binance.Rest.HTTPClient do
end
end

def signed_request_binance(url, params, method) do
def signed_request_binance(api_key, secret_key, url, params, method) do
Copy link
Owner

Choose a reason for hiding this comment

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

Same here

argument_string
)
|> Base.encode16()

body = "#{argument_string}&signature=#{signature}"

request_binance(url, body, method)
request_binance(api_key, url, body, method)
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah this is a bit odd

Probably better to make it a function with different signature or optional arg

@fbettag
Copy link
Contributor Author

fbettag commented Nov 2, 2023

Since i really don't know how you like the functions to be, i'd appreciate it if you changed the 3 signatures to your liking :)

I wouldn't be mad if you take over from here, i don't wanna screw around in your awesome code more than needed, but as for the test i suggest using get_account_summary to test this out.

Also, big kudos for the code generator. I've used that part of the library to show off to clients how awesome Elixir can be. ;)

@fbettag
Copy link
Contributor Author

fbettag commented Nov 2, 2023

actually trading (buy/sell) seems to be broken with this approach (worked with the other for some weird reason)

@dvcrn
Copy link
Owner

dvcrn commented Nov 3, 2023

I've changed the function signatures and shuffled things around, then added a test

Could you give this branch a try? #107

@dvcrn
Copy link
Owner

dvcrn commented Nov 11, 2023

Ping :)

@fbettag
Copy link
Contributor Author

fbettag commented Nov 15, 2023

sorry i'm currently very busy with pre-christmas work -.- everyone needs stuff launched before christmas

@fbettag
Copy link
Contributor Author

fbettag commented Nov 15, 2023

will try to get to it until the weekend

@fbettag
Copy link
Contributor Author

fbettag commented Nov 17, 2023

hey dude,

sorry for the delay. so with this branch i get the following when trying to buy or sell:

{:binance_error, %{code: -1104, msg: "Not all sent parameters were read; read '5' parameter(s) but was sent '6'."}}

@dvcrn
Copy link
Owner

dvcrn commented Nov 18, 2023

Thanks for the feedback

hrrrm, which method did you use? Can you post the code?

@fbettag
Copy link
Contributor Author

fbettag commented Nov 18, 2023

 {:binance_error,
  %{
    code: -1104,
    msg: "Not all sent parameters were read; read '5' parameter(s) but was sent '6'."
  }}}
[error] [Strategy 2] -- Error placing sell order for 2.27 of SOLBTC at price market: {:binance_error, %{code: -1104, msg: "Not all sent parameters were read; read '5' parameter(s) but was sent '6'."}}

  def sell(api_key, secret_key, symbol, money, price \\ nil) do
    type = if price == nil, do: "MARKET", else: "LIMIT"
    opts = [qty: money, api_key: api_key, secret_key: secret_key]
    opts = if price != nil, do: Keyword.put(opts, :price, price), else: opts

    Binance.Trade.post_order(symbol, "SELL", type, opts)
    |> IO.inspect(label: "binance sell order")
  end

@fbettag
Copy link
Contributor Author

fbettag commented Nov 18, 2023

ah damn now i see it, one second, that's actually my error. i just saw it by rereading the two things close together doooooh

Edit: nope this actually looks fine when dumping the params. had something else in my head apparently (sorry it's a saturday after having worked the past 3 weeks straight)

I also remember having this issue when i tried your approach. i have no idea how it triggers this issue.

@dvcrn dvcrn merged commit 5a57f82 into dvcrn:master Sep 2, 2024
7 checks passed
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