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

Jump to bottom

Check units when issuing assets #249

Merged
merged 2 commits into from
Aug 23, 2018
Merged

Check units when issuing assets #249

merged 2 commits into from
Aug 23, 2018

Conversation

blondfrogs
Copy link
Contributor

  • Move unit checking to own method
  • Create unit test for checking units
  • Add unit checking to asset issuance
  • Update reissue and transfer unit checking to use unit checking method

@blondfrogs blondfrogs requested a review from cfox August 23, 2018 16:11
cfox
cfox approved these changes Aug 23, 2018
View reviewed changes
Copy link
Contributor

@cfox cfox left a comment

Choose a reason for hiding this comment

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

Looks good. Couple of minor suggestions.

src/assets/assets.cpp Outdated
if (units < 0 || units > 8)
strError = "Invalid parameter: units must be between 0-8.";
if (nAmount > MAX_MONEY) {
strError = "Invalid parameter: asset amount greater than max.";
Copy link
Contributor

Choose a reason for hiding this comment

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

could put MAX_MONEY into strError

src/assets/assets.cpp Outdated
if (nHasIPFS != 0 && nHasIPFS != 1)
strError = "Invalid parameter: has_ipfs must be 0 or 1.";
if (!CheckAmountWithUnits(nAmount, units)) {
strError = "bad-txns-transfer-asset-amount-not-match-units";
Copy link
Contributor

Choose a reason for hiding this comment

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

is transfer a cpasta? generalize message? also not in the style of the others -- maybe reuse the amount must be divisable... from below?

@blondfrogs blondfrogs merged commit 16ed3ca into develop2 Aug 23, 2018
@blondfrogs blondfrogs deleted the check_units branch September 21, 2018 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfox cfox cfox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@blondfrogs @cfox