Conversation
karlb
left a comment
karlb
left a comment
There was a problem hiding this comment.
Semantics look fine. Just a bunch of stylistic suggestions. I'll give it a second read later.
crates/celo-revm/src/handler.rs
Outdated
There was a problem hiding this comment.
Should we use a saturating_sub here? An underflow is an error that we ignore this way.
There was a problem hiding this comment.
In theory, that has to be validated before. In this instance you already know that the effective_gas_price is at least the base_fee.
I'll double check if that's addressed before
There was a problem hiding this comment.
Ok, nice. This made me found a possible bug. As I said, that validation is performed before. But for every type except the cip64.
So I have to add that validation, but in this place it would be safe to use it like this
There was a problem hiding this comment.
Maybe we can find a tx-related place to put this information instead of the block env. I'll see if I find something to suggest.
There was a problem hiding this comment.
Maybe as a new field in the CeloTransaction?
There was a problem hiding this comment.
I was able to do it. As the tx is not mutable in the context, I had to clone and set it a few times, but I think it keeps everything cleaner
gastonponti
force-pushed
the
gastonponti/system-txs-gas-cap
branch
from
93e5102 to
7cc9257
Compare
| // Store CIP64 transaction information by modifying the transaction | ||
| let mut tx = evm.ctx().tx().clone(); | ||
| tx.cip64_tx_info = Some(Cip64Info { | ||
| actual_intrinsic_gas_used: gas_used, | ||
| logs_pre: logs, | ||
| logs_post: Vec::new(), | ||
| }); | ||
| evm.ctx().set_tx(tx); |
There was a problem hiding this comment.
Apparently the tx is not meant to be mutated or we would have a tx_mut_ref in the ContextTr. Not ideal, but I still like it better than putting it into the block context.
There was a problem hiding this comment.
Yeah, same. It's not forcing everything to be mutable at least, so we know that is only happening for this. But I agree that is cleaner than having info of the executed tx (only for one type) in the block_env
3 participants
[Depends on #60]
Implementation of the gas cap for debit/credit with a cip64 tx
Another possible solution was to reimplement the
SystemCallEvmand change thetransact_system_callto receive the gas_limit (or even create a new one), but I wanted to avoid changing more files from upstream if the we had a different solution that it's also cleanAlso:
Fixes https://github.com/celo-org/celo-blockchain-planning/issues/1201