sc process audit - 1 #2647
Merged
sc process audit - 1 #2647
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added unit Tests for SCPRocessor new and changeGasSchedule
# Conflicts: # integrationTests/vm/testInitializer.go
# Conflicts: # process/smartContract/process.go
# Conflicts: # process/smartContract/process.go
mihaib79
reviewed
Jan 18, 2021
sasurobert
reviewed
Jan 18, 2021
process/smartContract/process.go
Outdated
| @@ -641,6 +644,8 @@ func (sc *scProcessor) computeTotalConsumedFeeAndDevRwd( | |||
| } | |||
|
|
|||
| if !sc.flagDeploy.IsSet() { | |||
| //TODO: check if the totalFee is computed correctly for backwardsCompatible | |||
There was a problem hiding this comment.
yes it is. it does not matter here.
gas price modifier code and logic is handled in economicsData
process/smartContract/process.go
Outdated
| @@ -1214,6 +1219,7 @@ func (sc *scProcessor) addBackTxValues( | |||
|
|
|||
| // DeploySmartContract processes the transaction, than deploy the smart contract into VM, final code is saved in account | |||
| func (sc *scProcessor) DeploySmartContract(tx data.TransactionHandler, acntSnd state.UserAccountHandler) (vmcommon.ReturnCode, error) { | |||
| // TODO: check return 0 and err is ok? 0 is vmCommon.OK | |||
process/smartContract/process.go
Outdated
| @@ -1264,6 +1270,7 @@ func (sc *scProcessor) DeploySmartContract(tx data.TransactionHandler, acntSnd s | |||
| return vmcommon.UserError, sc.ProcessIfError(acntSnd, txHash, tx, err.Error(), []byte(""), snapshot, vmInput.GasLocked) | |||
| } | |||
|
|
|||
| //TODO: check why userError in the following ifs and not vmoutput.ReturnCode | |||
There was a problem hiding this comment.
vmOutput may be nil in case the VM returns nil, err
process/smartContract/process.go
Outdated
| @@ -1551,6 +1559,7 @@ func (sc *scProcessor) createSCRsWhenError( | |||
| consumedFee.Sub(consumedFee, moveBalanceCost) | |||
| } | |||
| } else { | |||
| //TODO: just AsynchronousCall here? No need for AsynchronousCallBack? | |||
process/transaction/baseProcess.go
Outdated
| @@ -150,6 +150,7 @@ func (txProc *baseTxProcessor) checkTxValues( | |||
| txFee = txProc.economicsFee.ComputeTxFee(tx) | |||
| } | |||
|
|
|||
| // TODO: check this: if checks the txFee against the balance before the txFee is finalized on the next if | |||
There was a problem hiding this comment.
this should stay is it is for backward compatibility reasons for the period until flagPenalizedTooMuchGas is not sent
| @@ -333,6 +333,7 @@ func (inTx *InterceptedTransaction) SenderAddress() []byte { | |||
|
|
|||
| // Fee returns the estimated cost of the transaction | |||
| func (inTx *InterceptedTransaction) Fee() *big.Int { | |||
| //TODO: check this, also no: !txProc.flagPenalizedTooMuchGas.IsSet() | |||
There was a problem hiding this comment.
no. this is correct. this was never changed
process/transaction/shardProcess.go
Outdated
| @@ -256,6 +256,8 @@ func (txProc *txProcessor) executingFailedTransaction( | |||
| return nil | |||
| } | |||
|
|
|||
| //TODO: check here, there is no if !flagPenalizedTooMuchGas.IsSet(): | |||
| // could it happen that not all taken fee or too much is taken from the sender account? | |||
There was a problem hiding this comment.
this is correct. this was never changed. it should stay as it is.
process/smartContract/process.go
Outdated
| @@ -1534,6 +1541,7 @@ func (sc *scProcessor) createSCRsWhenError( | |||
| } | |||
|
|
|||
| accumulatedSCRData := "" | |||
| // TODO: check why just ESDTTransfer here. No problem with burn, freeze? Can't they be crossShard? | |||
There was a problem hiding this comment.
the return data in case of burn and freeze is handled otherwise. The tokens are sent back through ESDTTransfer in case of error from metachain sc.
ccorcoveanu
previously approved these changes
Jan 18, 2021
process/smartContract/process.go
Outdated
| @@ -193,12 +193,15 @@ func (sc *scProcessor) GasScheduleChange(gasSchedule map[string]map[string]uint6 | |||
| defer sc.mutGasLock.Unlock() | |||
|
|
|||
| apiCosts := gasSchedule[core.ElrondAPICost] | |||
There was a problem hiding this comment.
Not for now, but this function could grow in the future, we should either make a structure from gasSchedule map[string]map[string]uint64, or at least have a simple function that fetches or checks nested maps.
mihaib79
previously approved these changes
Jan 18, 2021
mihaib79
approved these changes
Jan 18, 2021
ccorcoveanu
approved these changes
Jan 18, 2021
LucianMincu
approved these changes
Jan 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
unit tests for scProcess - Deploy, Execute and some BuiltInFunctions