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

Fixed SystemStackError #18

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

olhor
Copy link

@olhor olhor commented Nov 28, 2019

Fix: calling "indirect_rate" method when rates are not defined throws SystemStackError: stack level too deep.

@shailesh-kalamkar
Copy link

Thanks, @olhor for working on the solution.

@rmustafin It would be really helpful if you can review this PR as soon as possible. We are about to deploy this on production and don't want to run into this error. ( We are using latest 1.1.3 version)

Thanks in advance.

Fix: calling "indirect_rate" method when rates are not defined throws SystemStackError: stack level too deep.
@olhor olhor force-pushed the fix-indirect-rate branch from fc5b8d6 to 66e74bb Compare February 12, 2020 10:25
@shailesh-kalamkar
Copy link

shailesh-kalamkar commented Feb 12, 2020

@olhor Tried to use the solution provided here and there is a possible issue with the implementation.

Using the gem with Ruby Money gem like this.

Money.from_amount(100, 'USD', russian_central_bank_object).exchange_to('AED').to_f

When the rate is not available then it should throw Money::Bank::UnknownRate exception but it returning a value 0.

@shailesh-kalamkar
Copy link

I will try to check what we can do to fix it. Thanks.

@olhor
Copy link
Author

olhor commented Feb 12, 2020

@olhor Tried to use the solution provided here and there is a possible issue with the implementation.

Using the gem with Ruby Money gem like this.

Money.from_amount(100, 'USD', russian_central_bank_object).exchange_to('AED').to_f

When the rate is not available then it should throw Money::Bank::UnknownRate exception but it returning a value 0.

This is what I get:

Money.from_amount(100, 'USD', bank).exchange_to('AED').to_f
[WARNING] The default rounding mode will change from `ROUND_HALF_EVEN` to `ROUND_HALF_UP` in the next major release. Set it explicitly using `Money.rounding_mode=` to avoid potential problems.
Money::Bank::UnknownRate: No conversion rate known for 'USD' -> 'AED'

Are there any prerequisites? Should the rates be present?

@shailesh-kalamkar
Copy link

shailesh-kalamkar commented Feb 12, 2020

@olhor Oh 🤔

Are there any prerequisites? Should the rates be present?

Doing it like this -

bank_object = Money::Bank::RussianCentralBank.new
bank_object.update_rates

Money.from_amount(100, 'USD', bank_object).exchange_to('AED').to_f

divider = original_get_rate('RUB', from)
return nil if dividend.nil? || divider.nil?

dividend.to_f / divider.to_f

Choose a reason for hiding this comment

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

@olhor Can we trim it down to this ?

def indirect_rate(from, to)
  get_rate('RUB', to) / get_rate('RUB', from)
  dividend = original_get_rate('RUB', to)
  divider = original_get_rate('RUB', from)
  
  dividend && divider && dividend.to_f / divider.to_f
end

Copy link
Author

Choose a reason for hiding this comment

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

Line get_rate('RUB', to) / get_rate('RUB', from) will throw an exception if get_rate returns nil.

Choose a reason for hiding this comment

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

@olhor I am sorry. I missed to remove that line -

def indirect_rate(from, to)
  dividend = original_get_rate('RUB', to)
  divider = original_get_rate('RUB', from)
  
  dividend && divider && dividend.to_f / divider.to_f
end

Copy link
Author

Choose a reason for hiding this comment

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

No problem for me.

@shailesh-kalamkar
Copy link

@olhor Really appreciate your work and communication to resolve the issue. Thank you.

@shailesh-kalamkar
Copy link

@rmustafin Please check.

@arpit1094
Copy link

@olhor any chance we can merge this ?

@olhor
Copy link
Author

olhor commented Mar 20, 2024

@arpit1094 Don't ask me. I am not a maintainer, I do not have write access.

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.

3 participants