avoid evm.call for SGT
#10
Conversation
|
Should we use SoulGasToken contract to test? This will make sure the storage layout in test is consistent with op-geth in prod. |
It's a good idea, but seems |
I think you can create a test contract which inherts SoulGasToken with default ctor parameter. |
OK, I was thinking of using the exact same contract for testing. |
core/state_transition.go
Outdated
| } | ||
| const ( | ||
| // should keep it in sync with the balances field of SoulGasToken contract | ||
| balancesSlot = uint64(51) |
There was a problem hiding this comment.
I am thinking whether we can get the slot by an external call as
function getSlot() public pure returns (uint256) {
uint256 slot;
assembly {
slot := _balances.slot
}
return slot;
}
but definitively, it requires a lot of changes of code
There was a problem hiding this comment.
Gotcha, I think we can add a test in the monorepo that automatically checks for this.
core/state_transition.go
Outdated
| return parseSoulBalanceResp(ret) | ||
| } | ||
| var value common.Hash | ||
| current.Sub(current, amount).FillBytes(value[:]) |
There was a problem hiding this comment.
Can we use uint256.MustFromBig(current.Sub(current, amount)).Bytes32()? This will address the uncertainty of the endianness issue.
There was a problem hiding this comment.
Great suggestion! Done, and the test also changed to Bytes32().
| if err != nil { | ||
| return fmt.Errorf("GetSoulBalance error:%v", err) | ||
| } | ||
| have := st.GetSoulBalance(st.msg.From) | ||
| if have, want := have.ToBig(), new(big.Int).Sub(balanceCheck, st.msg.Value); have.Cmp(want) >= 0 { | ||
| if have, want := st.state.GetBalance(st.msg.From).ToBig(), st.msg.Value; have.Cmp(want) < 0 { |
There was a problem hiding this comment.
Does this mean that even if the sender has enough soul gas, it will still return an error if there isn’t enough native gas token?
There was a problem hiding this comment.
No, that issue is to be fixed by modifying ValidateTransactionWithState to also consider SGT balance. I'll create a separate PR after merging this one.
There was a problem hiding this comment.
In what situation that the msg is from 0x0 address?
This PR addresses this issue.
The related test(mocked) is here.