Feature/istanbul #3351
Feature/istanbul #3351
Conversation
…va-tron into feature/istanbul
…into feature/istanbul * 'feature/istanbul' of github.com:tronprotocol/java-tron: refine sonar enable change delegation
…va-tron into feature/istanbul
…into feature/istanbul # Conflicts: # framework/src/test/java/org/tron/common/runtime/vm/TRC10Test.java # framework/src/test/java/org/tron/common/runtime/vm/UnStakeTest.java
| } | ||
| break; | ||
| } | ||
| case ALLOW_TVM_ASSET_ISSUE: { |
There was a problem hiding this comment.
There is another proposal called ALLOW_TVM_TRANSFER_TRC10.
Should this be called ALLOW_TVM_TRC10_ISSUE in proposal scope.
|
|
||
| REWARDBALANCE(0xd8, 1, 1, OpCode.Tier.ExtTier), | ||
|
|
||
| ISSRCANDIDATE(0xd9, 1, 1, OpCode.Tier.ExtTier), |
There was a problem hiding this comment.
better call it ISWITNESS.
The code base has no sr candidate but witness candidate.
There was a problem hiding this comment.
ISWITNESS will be used in future plan. We define it as a different meaning compare to this scenario.
There was a problem hiding this comment.
It already introduces inconsistency.
| accountCapsule.setInstance(accountCapsule.getInstance().toBuilder().build()); | ||
| // spend 1024trx for assetissue, send to blackhole address | ||
| AccountCapsule bhAccountCapsule = repository.getAccount(repository.getBlackHoleAddress()); | ||
| bhAccountCapsule.setBalance(Math.addExact(bhAccountCapsule.getBalance(), TOKEN_ISSUE_FEE)); |
There was a problem hiding this comment.
TOKEN_ISSUE_FEE is not a constant. It's a chain parameter in dynamicProperty and is modified through proposals.
There was a problem hiding this comment.
Good point. Will refine this code in next release due to code freezing :)
| public DataWord isContract(DataWord address) { | ||
| ContractCapsule contract = getContractState() | ||
| .getContract(TransactionTrace.convertToTronAddress(address.getLast20Bytes())); | ||
| return contract != null ? new DataWord(1) : new DataWord(0); | ||
| } | ||
|
|
||
| public DataWord isSRCandidate(DataWord address) { |
There was a problem hiding this comment.
To be consistent with isContract(ISCONTRACT) opcode, you should use the name/OpCode/Solidity function name isWitness(ISWITNESS)`
- Contract/Witness/Normal is the type code of AccountType
- consistency across the whole TVM design really matters
|
|
||
| ISSRCANDIDATE(0xd9, 1, 1, OpCode.Tier.ExtTier), | ||
|
|
||
| TOKENISSUE(0xda, 1, 1, Tier.HighTier), |
There was a problem hiding this comment.
Since all other OpCodes are all verb-phrase, should this be ISSUETOKEN?
There was a problem hiding this comment.
Will consider to rename the opcode in next release due to code freezing.
There was a problem hiding this comment.
I know that if it is released, then It will never be changed.
So sad that your release plan is more important that code and consistency.
There was a problem hiding this comment.
It can be changed. Not a hard fork case. As long as we keep the opcode hex value the same in the interpreter, it will not influence the transaction behavior and the solidity code. Just a naming issue.
What does this PR do?
Why are these changes required?
This PR has been tested by:
Follow up
Extra details