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.

Sign up for GitHub

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

Accept all valid currency codes in API #4566

Merged
merged 4 commits into from
Sep 22, 2023
Merged

Accept all valid currency codes in API #4566

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3590c9
Accept all valid currency codes in API
thejohnfreeman Jun 9, 2023
74d5589
Add comment explaining pattern
thejohnfreeman Aug 14, 2023
0ec0461
Merge branch 'develop'
thejohnfreeman Aug 14, 2023
5eae600
Merge branch 'develop'
thejohnfreeman Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/ripple/net/impl/RPCCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ class RPCParser
static Json::Value
jvParseCurrencyIssuer(std::string const& strCurrencyIssuer)
{
static boost::regex reCurIss("\\`([[:alpha:]]{3})(?:/(.+))?\\'");
static boost::regex reCurIss(
"\\`([][:alnum:]<>(){}[|?!@#$%^&*]{3})(?:/(.+))?\\'");
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this regex working correctly? Is the tilde character (third character from the beginning) replaceable with the single-quote (last but one character) inside the regex?

can we include a comment referring to full alphabet or reuse the detail::isoCharSet variable? [I was thinking: "\`((detail::isoCharSet){3})(?:/(.+))?\'", but I'm not sure]

also, I've used regex libraries in other languages, but this one baffles me. what is the purpose of the suffix of this regular expression? [?:/(.+))?]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a boost::regex. It uses Perl syntax, which the library documents online. Look at the section "Buffer boundaries". (The character ` is called a backtick; tilde is ~.) \` matches the start of a buffer and \' matches the end of a buffer, so no, they cannot be replaced.

You could try to construct a regex string where detail::isoCharSet sits in a character class, but you would have to change the order within detail::isoCharSet because ] cannot appear in the middle of a character class; it can only appear as the first character in the class. I don't think that factoring is worth it, though.

(?:...) is a non-capturing group. It groups subatoms into a larger atom without creating a capture group. So (?:/(.+))? optionally matches a forward slash and a non-empty sequence of characters, capturing the characters after the slash into a group.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, this is helpful. thank you

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still feel adding a comment would be helpful for future readers of the code, but it's upto you.

On that note, how do you include suggested patches in github comments? I remember you did something like that for me a while ago.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"\\`([][:alnum:]<>(){}[|?!@#$%^&*]{3})(?:/(.+))?\\'");
// The below regex matches the pattern as specified in UIntTypes.cpp:detail::isoCharSet. It is utilizes boost::regex and adheres to the Perl syntax for regular expressions.
"\\`([][:alnum:]<>(){}[|?!@#$%^&*]{3})(?:/(.+))?\\'");

Copy link
Collaborator

Choose a reason for hiding this comment

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

@thejohnfreeman to accept, modify, or decline suggested comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've pushed an alternative comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok 👍


boost::smatch smMatch;

Expand Down
10 changes: 2 additions & 8 deletions src/ripple/protocol/impl/UintTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,8 @@ to_currency(Currency& currency, std::string const& code)

currency = beast::zero;

std::transform(
code.begin(),
code.end(),
currency.begin() + detail::isoCodeOffset,
[](auto c) {
return static_cast<unsigned char>(
::toupper(static_cast<unsigned char>(c)));
});
std::copy(
code.begin(), code.end(), currency.begin() + detail::isoCodeOffset);

return true;
}
Expand Down