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

chore: make validate currency as public method #1114

Merged
merged 5 commits into from
Jun 22, 2023

Conversation

jerrylaloan
Copy link
Contributor

@jerrylaloan jerrylaloan commented Jun 19, 2023

Description of the changes

The purpose of this change is to allow users to provide a custom validateCurrency & validateAddress method for currency which not supported by the library.
Updated:

  • method access as public
  • updated usage in the request-client

if (
currency.type === RequestLogicTypes.CURRENCY.ISO4217 ||
currency.type === RequestLogicTypes.CURRENCY.ETH ||
currency.type === RequestLogicTypes.CURRENCY.BTC
)
return true;
return this.validateAddress(currency.value, currency);
return CurrencyManager.validateAddress(currency.value, currency);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't validateAddress also be moved to an instance method rather than static? For the same reason you changed validateCurrency (ability to override behaviour)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's update validateAddress as well 👍

@coveralls
Copy link

coveralls commented Jun 19, 2023

Coverage Status

coverage: 87.341%. remained the same when pulling 04eab39 on make-validate-currency-as-public-method into f6a63d0 on master.

Copy link
Member

@alexandre-abrioux alexandre-abrioux left a comment

Choose a reason for hiding this comment

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

The same remark as @benjlevesque; other than that it looks good to me!

@jerrylaloan jerrylaloan merged commit d1f6d0d into master Jun 22, 2023
@jerrylaloan jerrylaloan deleted the make-validate-currency-as-public-method branch June 22, 2023 08:41
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.

5 participants