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

Bitcoin Feature #355

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

KhalilBellakrid
Copy link
Contributor

Support choosing change address path.

throw make_exception(api::ErrorCode::INVALID_ARGUMENT, "No change path provided by user.");
}
auto it = buddy->request.changePaths.back();
return std::stoul(it.substr(it.find_last_of('/') + 1), nullptr, 0);
Copy link
Contributor

@alekece alekece Oct 16, 2019

Choose a reason for hiding this comment

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

Which version of c++ we are using here ? Because you can remove the substring construction by using the new std::string_view.
More than that, I think there is a bug. If the last changePaths doesn't have any '/', find_last_of returns string::npos which is the maximum value to size_t so when you add 1 to that value, you got a 0 and your stoul call returns 0.
This is what you want ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s a C++17 feature. We can’t use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@phaazon Hence my question to @KhalilBellakrid ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use C++14 so .. SFYL ! :trollface:
Concerning the behaviour, yes this is exactly what I'm looking for, we want support all ways of setting paths, user can input 0/1/8, 1/8 or 8 and we will always retrieve the right index, if someone inputs 8/ or some other invalid path then we would throw an exception on the stoul hence the encapsulation in the Try and in that case we would fall back on previous behaviour (getting a fresh change address)

@hadronized
Copy link
Contributor

@KhalilBellakrid you got two approvals, so you can merge — unless you wanna investigate those failing builds.

@hadronized hadronized added the HODL Done, but don't merge yet ! label Mar 12, 2020
@hadronized
Copy link
Contributor

HODL for stalling PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement HODL Done, but don't merge yet !
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants