refactor sender transfer

nimbus/p2p/executor.nim

@@ -7,16 +7,7 @@ import options,
   ../vm/interpreter/vm_forks
 
 proc contractCall(tx: Transaction, vmState: BaseVMState, sender: EthAddress, forkOverride=none(Fork)): GasInt =
-  # TODO: this function body was copied from GST with it's comments and TODOs.
-  # Right now it's main purpose is to produce VM tracing when syncing block with
-  # contract call. At later stage, this proc together with applyCreateTransaction
-  # and processTransaction need to be restructured.
-
-  # TODO: replace with cachingDb or similar approach; necessary
-  # when calls/subcalls/etc come in, too.
   var db = vmState.accountDb
-  let storageRoot = db.getStorageRoot(tx.to)
-
   var computation = setupComputation(vmState, tx, sender, forkOverride)
   if execComputation(computation):
     let
@@ -27,11 +18,8 @@ proc contractCall(tx: Transaction, vmState: BaseVMState, sender: EthAddress, for
       gasRefundAmount = (gasRefund + gasRemaining).u256 * tx.gasPrice.u256
 
     db.addBalance(sender, gasRefundAmount)
-
     return (tx.gasLimit - gasRemaining - gasRefund)
   else:
-    db.addBalance(sender, tx.value)
-    db.setStorageRoot(tx.to, storageRoot)
     if computation.tracingEnabled: computation.traceError()
     vmState.clearLogs()
     return tx.gasLimit
@@ -68,7 +56,7 @@ proc processTransaction*(t: Transaction, sender: EthAddress, vmState: BaseVMStat
       balance -= upfrontGasCost
     transactionFailed = true
   else:
-    balance -= upfrontCost
+    balance -= upfrontGasCost
 
   db.setBalance(sender, balance)
   if transactionFailed:
@@ -89,7 +77,7 @@ proc processTransaction*(t: Transaction, sender: EthAddress, vmState: BaseVMStat
       if code.len == 0 and not isPrecompiles(t.to):
         # Value transfer
         trace "Transfer", value = t.value, sender, to = t.to
-
+        db.subBalance(sender, t.value)
         db.addBalance(t.to, t.value)
       else:
         # Contract call
@@ -99,13 +87,9 @@ proc processTransaction*(t: Transaction, sender: EthAddress, vmState: BaseVMStat
         # TODO: Run the vm with proper fork
         return contractCall(t, vmState, sender)
 
-  if gasUsed > t.gasLimit:
-    gasUsed = t.gasLimit
-
+  if gasUsed > t.gasLimit: gasUsed = t.gasLimit
   var refund = (t.gasLimit - gasUsed).u256 * t.gasPrice.u256
-  if transactionFailed:
-    refund += t.value
-
+  if transactionFailed: refund += t.value  
   db.addBalance(sender, refund)
   return gasUsed
 

nimbus/vm_state_transactions.nim

@@ -67,6 +67,7 @@ proc execComputation*(computation: var BaseComputation): bool =
   defer: snapshot.dispose()
 
   computation.vmState.mutateStateDB:
+    db.subBalance(computation.msg.origin, computation.msg.value)
     db.addBalance(computation.msg.storageAddress, computation.msg.value)
 
   try:
@@ -124,13 +125,7 @@ proc applyCreateTransaction*(tx: Transaction, vmState: BaseVMState, sender: EthA
     db.addBalance(sender, (tx.gasLimit - gasUsed - codeCost + gasRefund).u256 * tx.gasPrice.u256)
     return (gasUsed + codeCost - gasRefund)
   else:
-    # FIXME: don't do this revert, but rather only subBalance correctly
-    # the if transactionfailed at end is what is supposed to pick it up
-    # especially when it's cross-function, it's ugly/fragile
-    var db = vmState.accountDb
-    db.addBalance(sender, tx.value)
-    if c.tracingEnabled:
-      c.traceError()
+    if c.tracingEnabled: c.traceError()
     vmState.clearLogs()
     return tx.gasLimit
 

tests/test_generalstate_json.nim

@@ -50,7 +50,7 @@ proc testFixtureIndexes(prevStateRoot: Hash256, header: BlockHeader, pre: JsonNo
   let gasCost = transaction.gasLimit.u256 * transaction.gasPrice.u256
   vmState.mutateStateDB:
     db.incNonce(sender)
-    db.subBalance(sender, transaction.value + gasCost)
+    db.subBalance(sender, gasCost)
 
   if transaction.isContractCreation and transaction.payload.len > 0:
     vmState.mutateStateDB:
@@ -83,9 +83,6 @@ proc testFixtureIndexes(prevStateRoot: Hash256, header: BlockHeader, pre: JsonNo
     # TODO: only here does one commit, with some nuance/caveat
   else:
     vmState.mutateStateDB:
-      # XXX: the coinbase has to be committed; the rest are basically reverts
-      db.addBalance(sender, transaction.value)
-      db.setStorageRoot(transaction.to, storageRoot)
       db.addBalance(header.coinbase, gasCost)
 
 proc testFixture(fixtures: JsonNode, testStatusIMPL: var TestStatus) =