-
Notifications
You must be signed in to change notification settings - Fork 165
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
FIP0054: Simplify and expand CALL logic #607
Conversation
We no longer special-case depending on the target actor type in CALL and have changed the logic around solidity "transfers" to CALL with 10M Filecoin Gas. I've also taken the opportunity to expand on the other call variants a bit, and clarify how Filecoin "send" parameters are derived.
b9d2fb6
to
b2c414e
Compare
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.
(This is a peer review and not an editor review. As an editor, I would merge this patch.)
3. If input data is treated as `IPLD_RAW` if non-empty, or the empty block (block `0`) if empty. | ||
4. The value is treated as a Filecoin token amount. | ||
5. The gas limit is computed as follows: | ||
1. If the value zero and the gas limit is 2300, or the gas limit is zero and the value is non-zero, FEVM sets the gas limit to 10M. This ensures that bare "transfers" continue to work as 2300 isn't enough gas to perform a call on Filecoin. 10M was picked to be enough to "always work" but not so much that it becomes a security concern. |
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.
If we're finally going in this direction (static translation of gas limit), it would make sense to add a point in Design rationale about having to maintain this equivalent eternally, as:
a) the gas model evolves release over release
b) the EVM runtime evolves and adds/removes overhead
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 would make sense to add a point in Design rationale about having to maintain this equivalent eternally, as
I'll expand on it. The goal is to pick a number such that that it will never be an issue.
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.
I'm also happy to pick a higher number, like 100M. That would be 1% of the maximum block size.
FIPS/fip-0054.md
Outdated
`DELEGATECALL` behaves differently depending on the recipient: | ||
|
||
1. If the target is a precompile address, it is handled internally according to the precompile logic defined in `CALL` above. | ||
2. If the target actor is an EVM runtime actor, it fetches the bytecode of the target and calls `InvokeContractDelegate` on itself to create a new call stack "frame" whilst still operating on our own state. |
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.
If the target actor is an EVM runtime actor
- Describe how "is an EVM runtime actor" is tested.
- How does it fetch bytecode? I assume by calling GETBYTECODE or something. Does the code come back inline? Does it get a CID? A block handle (have we invoked visibility here)?
FIPS/fip-0054.md
Outdated
1. If the target is a precompile address, it is handled internally according to the precompile logic defined in `CALL` above. | ||
2. If the target actor is an EVM runtime actor, it fetches the bytecode of the target and calls `InvokeContractDelegate` on itself to create a new call stack "frame" whilst still operating on our own state. | ||
3. If the target is an account, placeholder, or an EthAccount actor ([FIP-0055]), it returns nothing and success (pushes 1 onto the stack). | ||
4. If the target is any other type of actor, it returns nothing and failure (pushes 0 onto the stack). |
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.
Discussion here about avoiding this remaining target-code-dependent behaviour switching.
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.
Approving this diff as an editor, though I still have a technical discussion point open
We no longer special-case depending on the target actor type in CALL and have changed the logic around solidity "transfers" to CALL with 10M Filecoin Gas.
I've also taken the opportunity to expand on the other call variants a bit, and clarify how Filecoin "send" parameters are derived.