-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(acurator): add more check for AssetIssueActuator #6525
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
fix(acurator): add more check for AssetIssueActuator #6525
Conversation
actuator/src/main/java/org/tron/core/actuator/AssetIssueActuator.java
Outdated
Show resolved
Hide resolved
protocol/src/main/protos/core/contract/asset_issue_contract.proto
Outdated
Show resolved
Hide resolved
| int64 total_supply = 4; | ||
| repeated FrozenSupply frozen_supply = 5; | ||
| int32 trx_num = 6; | ||
| int32 trx_num = 6; // The fields trx_num and num are only used to calculate price and avoid usage of decimal, asset price = trx_num / num |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using // The fields trx_num and num define the exchange rate: num tokens can be purchased with trx_num TRX. This avoids using decimals. Example: trx_num=1, num=100 means 1 TRX buys 100 tokens. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to use example in proto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so what about using // The fields trx_num and num define the exchange rate: num tokens can be purchased with trx_num TRX. This avoids using decimals..
Consider using exchange rate instead of price—it might be more precise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
| } | ||
|
|
||
| @Test | ||
| public void SameTokenNameStartTimeTooBig() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the uTest name is SameTokenNameStartTimeTooBig? what about add this check in issueTimeTest() above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a hard fork condition, so use a new method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change the name as issueStartTimeTooBig.
waynercheung
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| if (chainBaseManager.getForkController().pass(ForkBlockVersionEnum.VERSION_4_8_1)) { | ||
| long frozenPeriod = next.getFrozenDays() * FROZEN_PERIOD; | ||
| try { | ||
| LongMath.checkedAdd(assetIssueContract.getStartTime(), frozenPeriod); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use StrictMath.addExact ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LongMath.checkedAdd and StrictMath.addExact's implementations are same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer Math.addExact over LongMath.checkedAdd: both check for long overflow, but addExact is JDK-native, avoids Guava dependency, and ensures a single consistent implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use StrictMathWrapper.addExact instead.
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details