-
Notifications
You must be signed in to change notification settings - Fork 4
add allowSgtValue/disallowSgtValue to SGT contract #89
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
add allowSgtValue/disallowSgtValue to SGT contract #89
Conversation
|
Is there an issue to track the motivation and background for this? Additionally, in many cases, the EthStorage contract might be called by a dApp contract to store data, rather than being the direct transaction target. Can we also support this scenario? |
I updated the description above.
@qizhou What's your opinion? The implementation will be much trickier if we want to support indirect caller. I'm not sure if we should give SoulGasToken so much flexibility. |
|
I pushed the tests in this PR, since it depends on the contract change. |
The indirect caller is tricker, but Qiang is right: the EthStorage contract is mostly called by the contracts. One solution that comes to my mind is:
|
I see how it works, but the trust model is a bit different from ethereum's convention in that evil contracts can upgrade and spend user's SGT balance without permission. But since the permission is whitelisted, this concern should be controllable since we can revoke the whitelist if bad things happen. |
qizhou
left a comment
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.
LGTM. This seems to be simplest change to allow storage contract to charge SGT without changing op-geth.
| if (balance >= amount) { | ||
| amountCharged = amount; | ||
| } else { | ||
| amountCharged = balance; |
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.
It seems a bit odd that if the amount to be charged exceeds the balance, we still attempt to charge it instead of returning 0.
How will the EthStorage contract call this method? Based on my understanding, the EthStorage contract will first check if msg.value is sufficient. If not, it will call chargeFromOrigin(amountNeeded - msg.value). If the balance is still insufficient, it will revert.
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.
chargeFromOrigin may charge less than requested, and the remaining amount is expected to be charged from native balance by the caller contract. The caller contract can revert if it can't charge from native balance.
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.
Yes, I prefer charging SGT first in EthStorage and then charging native balance so that it may provider better UX.
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.
A new issue has been opened to track the corresponding changes: ethstorage/storage-contracts-v1#122
This PR prepares
SoulGasTokencontract so that if:IS_BACKED_BY_NATIVEis truetx.tothen
SoulGasTokenbalance can be used asmsg.value.