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

Ametsuchi: fixed asset quantity overflow detection #154

Merged

Conversation

MBoldyrev
Copy link
Contributor

@MBoldyrev MBoldyrev commented Jul 3, 2019

Description of the Change

Prior to this change, the maximum asset quantity was computed as 2^(256-p), where p is the asset precision. This had a weird consequence that an asset with greater p would have a greater significand* limit that will not fit a 256-bit integer.


*) significand means M in N = M * e ^ p floating point notation

Benefits

Now the мантисса limit is always uint256_max, regardless the asset precision. Also the involved SQL got prettified (to my taste).

Possible Drawbacks

Usage Examples or Tests [optional]

See the *Overflow* tests in add_asset_qty_test and postgres_executor_test.

Alternate Designs [optional]

@MBoldyrev MBoldyrev self-assigned this Jul 3, 2019
@MBoldyrev MBoldyrev mentioned this pull request Jul 4, 2019
Copy link
Contributor Author

@MBoldyrev MBoldyrev left a comment

Choose a reason for hiding this comment

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

Fixed

irohad/ametsuchi/impl/postgres_command_executor.cpp Outdated Show resolved Hide resolved
@MBoldyrev MBoldyrev force-pushed the fix/asset-quantity-overflow branch from fdf45fb to b42a0e7 Compare July 19, 2019 11:53
@@ -39,12 +40,19 @@ namespace common_constants {
extern const std::string kSameDomainUserId;
extern const std::string kAnotherDomainUserId;
extern const std::string kAssetId;
extern const std::string kAnotherDomainAssetId;
Copy link
Contributor

Choose a reason for hiding this comment

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

kAnotherDomainAssetId is unused and can be removed.


// misc
extern const shared_model::interface::Amount
kAmountPrec1Max; // maximum amount of asset with pricision 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
kAmountPrec1Max; // maximum amount of asset with pricision 1
kAmountPrec1Max; // maximum amount of asset with precision 1

same below

AND precision >= $3

UNION
SELECT 4, value < (2::decimal ^ 256) / 10::decimal ^ precision
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SELECT 4, value < (2::decimal ^ 256) / 10::decimal ^ precision
SELECT 4, value < (2::decimal ^ 256) / (10::decimal ^ precision)

* @given two users with all required permissions, one having the maximum
* allowed quantity of an asset with precision 1
* @when execute a tx from another user with TransferAsset command for that
* asset with the smallest possible quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* asset with the smallest possible quantity
* asset with the smallest possible quantity and then with a lower
* precision

* allowed quantity of an asset with precision 1
* @when execute a tx from another user with TransferAsset command for that
* asset with the smallest possible quantity
* @then that transaction is not committed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @then that transaction is not committed
* @then the last two transactions are not committed

* @given two users with all required permissions, one having the maximum
* allowed quantity of an asset with precision 2
* @when execute a tx from another user with TransferAsset command for that
* asset with the smallest possible quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* asset with the smallest possible quantity
* asset with the smallest possible quantity and then with a lower
* precision

@MBoldyrev MBoldyrev requested a review from lebdron July 24, 2019 14:40
@MBoldyrev MBoldyrev force-pushed the fix/asset-quantity-overflow branch from 932705a to 59b25d0 Compare July 30, 2019 15:36
Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
Signed-off-by: Mikhail Boldyrev <miboldyrev@gmail.com>
@MBoldyrev MBoldyrev force-pushed the fix/asset-quantity-overflow branch from 59b25d0 to 970f1fd Compare July 31, 2019 04:50
@MBoldyrev MBoldyrev merged commit 74a6e48 into hyperledger-iroha:master Jul 31, 2019
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