rusty_money icon indicating copy to clipboard operation
rusty_money copied to clipboard

Account for inverse rate

Open JavedNissar opened this issue 4 years ago • 2 comments

This is a PR to address #51. Feel free to close.

JavedNissar avatar Jan 14 '21 00:01 JavedNissar

First off, thanks for submitting this, appreciate the PR!

Meta thoughts on this:

  1. What is the primary benefit of doing the ExchangeRateQuery over just building the logic into the Exchange in your opinion? I'm trying to justify the added complexity of another class here.
  2. On further reflection, forcing the inverse rate to always be set isn't desirable for all cases (e.g. currency spreads). I think this should be an optional flag on set_rate.
  3. Since this is a big API change, ill hold this back till the 0.5.0 release if we do decide to go forward with it.

You're welcome! Thanks for publishing this library, it's great! With regard to those meta thoughts:

  1. I think I covered this in a reply to one of your line comments
  2. Also in a line comment reply
  3. That's totally fine!

JavedNissar avatar Jan 16 '21 21:01 JavedNissar

@varunsrin Are you still interesting in having this PR merged in? If not, I can close it.

JavedNissar avatar Jan 29 '21 04:01 JavedNissar