[FVM] beyond EVM part 4 - adding EVM handler #4864
Conversation
ramtinms
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4864 +/- ##
==========================================
- Coverage 55.75% 52.30% -3.46%
==========================================
Files 959 701 -258
Lines 89270 63793 -25477
==========================================
- Hits 49773 33367 -16406
+ Misses 35752 27839 -7913
+ Partials 3745 2587 -1158
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
fvm/evm/handler/handler.go
Outdated
| func (h *ContractHandler) setupNewBlockDraft() { | ||
| lastExecutedBlock, err := h.blockchain.LatestBlock() | ||
| handleError(err) | ||
|
|
||
| parentHash, err := lastExecutedBlock.Hash() | ||
| if err != nil { | ||
| // this is a fatal error | ||
| panic(err) | ||
| } | ||
| h.newBlockDraft = &types.Block{ | ||
| Height: lastExecutedBlock.Height + 1, | ||
| ParentBlockHash: parentHash, | ||
| TotalSupply: lastExecutedBlock.TotalSupply, | ||
| TransactionHashes: make([]gethCommon.Hash, 0), | ||
| } | ||
| } |
There was a problem hiding this comment.
wouldn't this make sense to be part of the blockchain interface and implemented as part of the blocks? the block model could also implement all the methods below to update the defined below.
So basically changing this method to something like:
func (h *ContractHandler) blockDraft() {
if h.newBlockDraft != nil {
return h.newBlockDraft
}
h.newBlockDraft = h.blockchain.NewBlock()
}
And then removing all the methods below since you can just do:
h.blockDraft().AppendTxHash(hash)
There was a problem hiding this comment.
Also thinking about it again, it might even be useful for blockchain to handle that, so you could create a draft block using the blockchain like above, but then it would only allow you to create a new one after you commit it? That way you also don't have to check if nil here, but could just be a call to blockchain.DraftBlock
|
|
||
| // Balance returns the balance of this bridged account | ||
| func (a *Account) Balance() types.Balance { | ||
| ctx := a.fch.getBlockContext() |
There was a problem hiding this comment.
should accessing the balance be metered?
There was a problem hiding this comment.
for read calls, currently we don't meter any computation and only check the storage interaction limits
There was a problem hiding this comment.
we might want to do it in the future, I'll add a TODO
fvm/evm/handler/handler.go
Outdated
| res, | ||
| ), | ||
| ) | ||
| newBalance := a.fch.getBlockDraftTotalSupply() + v.Balance().ToAttoFlow().Uint64() |
There was a problem hiding this comment.
I guess overflow is not a real issue for us here?
There was a problem hiding this comment.
uint64 captures overflows.
fvm/evm/handler/handler.go
Outdated
| func (h *ContractHandler) commitBlockDraft() { | ||
| err := h.blockchain.AppendBlock(h.newBlockDraft) | ||
| handleError(err) | ||
| h.emitEvent(types.NewBlockExecutedEvent(h.newBlockDraft)) |
There was a problem hiding this comment.
I wonder if this could cause exceeding computation limit. I might be nitpicking, but could there be a case where the tx gas limit is the same as computation limit, so the checkGasLimit passes, and tx actually uses all the gas, but then emitting the event adds computation usage and causes the transaction to fail. I guess it could be ok since it would revert the transaction, but I rather ask than not.
There was a problem hiding this comment.
emitting events are metered through the FVM backend, so they would fail similarly to how cadence transactions fail.
| @@ -44,3 +43,9 @@ type Backend interface { | |||
| environment.Meter | |||
| environment.EventEmitter | |||
| } | |||
|
|
|||
| // AddressAllocator allocates addresses, used by the handler | |||
| type AddressAllocator interface { | |||
…to ramtin/evm-add-handler
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This PR adds the EVM handler, and here are the responsibilities of the handler