Cinderella-Man / hands-on-elixir-and-otp-cryptocurrency-trading-bot

Source code to generate the "Hands-on Elixir & OTP: Cryptocurrency trading bot" book
https://elixircryptobot.com
263 stars 34 forks source link

Chapter 2: Use gitignored secrets.exs for Binance keys #16

Closed mcintyre94 closed 3 years ago

mcintyre94 commented 3 years ago

Updated:

This PR updates the section where we set the Binance API keys in order to avoid accidentally checking these keys into source control. We put the Binance config in a new config/secrets.exs which is added to .gitignore and included in config/config.exsif it exists using import_config.


Originally:

I appreciate that this is maybe a bit opinionated, but I think it's worth showing a source-control-safe method of setting the keys for Binance. I'm following the book in a public repo and wouldn't want my keys in that repo. In this PR I'm using environment variables in config/runtime.exs. I've chosen to use runtime.exs in case the guide gets into compiling later on - compile time environment variable resolution is quite confusing/unexpected IMO. I've demonstrated setting these environment variables in the following test run.

Cinderella-Man commented 3 years ago

Hello :wave:

Thank you very much for creating this PR. I think you are right :+1: The book should not create a risk for people to commit/push passwords into a repository.

I was thinking about the solution that you proposed. However, as I want to go into "deployment" in one of the future chapters, I would prefer to stay away from the environmental variables(as part of the "deploying" chapter, I'm planning to introduce env vars).

Maybe a half-way solution would be introducing a config/secrets.exs file that git would ignore, and inside, it would have a normal/hardcoded key and secret for binance? (it would be explicitly ignored by git inside .gitignore). The config/dev.exs file would include it (if it exists).

I wouldn't need to introduce environmental variables, and we wouldn't risk people pushing their binance details to GitHub?

Please, let me know what do you think?

mcintyre94 commented 3 years ago

That’s definitely reasonable, and it’s cool to hear you plan to cover deployment too! Your solution definitely sounds good to me. Would you like me to take a first crack at that in this PR or would you rather write it? I’m happy either way :)

Cinderella-Man commented 3 years ago

Hi :wave:

Sorry for the late response - I'm off sick atm :face_with_thermometer: As you highlighted this issue, I don't want to take credit for it - could you please make a new draft/PR(or update this one). I will approve it, merge and apply further changes/updates myself if any will be required. Does this sound ok?

I was thinking about introducing the secrets.exs file at the moment when currently the Binance's access details are added to the configuration. Probably with an explanation why programmers should be smarter then me :smiley: and introduce that git ignored file - adding the file to git ignore should be mentioned as well probably - just throwing ideas there! :rocket: (my head doesn't work atm)

Please let me know?

Thanks in advance :+1:

mcintyre94 commented 3 years ago

Hey no worries, get better soon! That sounds good to me. I’ll update this PR and then tag you when it’s good for another look. Don’t rush to look though, take your time :)

Hope you feel better soon!

mcintyre94 commented 3 years ago

@Cinderella-Man I've updated this now to use the secrets.exs as suggested. I've also opened a companion PR against chapter 2 of the source code demonstrating these changes: https://github.com/frathon/hands-on-elixir-and-otp-cryptocurrency-trading-bot-source-code/pull/14

Cinderella-Man commented 3 years ago

Thank you very much for this PR (as well as the second one for the source code!) :heart:

Everything looks good - I just changed the target branch to 0.2.1, so it will be released with the new version.

I'm happy to merge this PR :+1: Thank you and congrats on becoming a contributor to the book! :heart: :rocket: