-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make ValidateTransactionWithState also consider sgt balance #12
make ValidateTransactionWithState also consider sgt balance #12
Conversation
if opts.L1CostFn != nil { | ||
if l1Cost := opts.L1CostFn(tx.RollupCostData()); l1Cost != nil { // add rollup cost | ||
cost = cost.Add(cost, l1Cost) | ||
} | ||
} | ||
if balance.Cmp(cost) < 0 { | ||
return fmt.Errorf("%w: balance %v, tx cost %v, overshot %v", core.ErrInsufficientFunds, balance, cost, new(big.Int).Sub(cost, balance)) | ||
if balance.Cmp(cost) < 0 && sgtBalance.Cmp(cost) < 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic is not consistent with execution logic where msg.from is 0x0 (
op-geth/core/state_transition.go
Line 342 in 25669f6
if st.msg.From != (common.Address{}) && st.evm.ChainConfig().IsOptimism() && st.evm.ChainConfig().Optimism.UseSoulGasToken { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it seems the check for zero account is no longer necessary after we avoided evm.call, so removed.
The UseSoulGasToken flag is added back.
Does this mean that if the sender’s native gas token balance is zero or insufficient to cover the gas fee, the transaction will always fail, regardless of the soul gas token balance? |
It will only fail when both native balance and sgt balance are not sufficient. |
what I ask is the condition before this PR |
Before this change, it only checks native balance, so yes. |
This PR fixes #11