Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update EIP-4788: initial stab at v2 #7456
Update EIP-4788: initial stab at v2 #7456
Changes from 14 commits
5cb47b4
68acfa5
2eee682
9f5179e
c4caa4a
9ae7e6f
45b5803
72edbee
6bf6dca
9fdfa7f
ec64413
26e6ae7
c02a3a9
998943a
a5a4047
3d048ad
bb28b8d
29cfc56
240f522
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
oh, funny. seems okay though
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 somewhat seems to imply that this contract has an ABI (which it has not, at least not in current assembly). I would really like to do the ABI-like approach as in lightclient/4788asm#5 since this will also be very helpful for solidity users (you can now
BeaconRootContract(address).get(timest)
.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 think it is a bad idea to enshrine solidity ABI behavior. It is ubiquitous today, but in the future I expect other language will have different calling conventions. Solidity should directly support this contract like they do for
ecrecover
.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 kind of want to make the case for avoiding a system-TX and just setting these storage values at the top of the block. Then the code is just
get
and we don't have to test/specify how to do system-TXsweaker held opinion than the deploy method
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 think if we're going to use a standard contract we should embrace it and allow it to have storing functionality.
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.
Wrote this already in discord, posting here too for visibility.
With the spec being 'system call', it means a reference client can just do a simple call. A client implementor can still choose not to, and instead bypass the whole call and update the values directly.
If the eip standardizes 'direct update', however, and omits the system-update path from the contract, then we remove all optionality, and force more special-case code into clients
Therefore I think the 'system call' approach is the best way forward.
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 think the EIP should mention that "direct update" is a valid/possible client implementation/optimization detail.
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.
system-tx would be a good generic functionality to have in long term imo
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 don't love this method with a fake signature and stuff...
Can we not just drop bytecode X at address Y at the fork?
e.g. like 1011 proposed -- https://eips.ethereum.org/EIPS/eip-1011#deploying-casper-contract
I think it's more straight forward to just place it exactly where we want it, rather than trying to use a TX of sorts but without gas or signature verification which requires a lot more exceptional logic
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.
and then we don't have to think about things like this.
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.
To be clear, this tx does still abide by gas and signature verification. The signature is constructed arbitrarily and therefore the sender is a one-time sender address (pk is not known).
--
In an effort to minimize protocol baggage, I think it is better to deploy the contract using standard methods. Once we are satisfied with the bytecode we'd like to deploy, it could be deployed by anyone. We simply have to agree on the address at which it is deployed. It doesn't have to be a synthetic tx, although I do slightly prefer this method as it is permissionless (anyone can fund the account and submit the tx).
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.
Why?
I would rather specify some hard limit, like
30M
gas, as suggested by @yperbasis . I suspect most actual implementation will need to set a limit anyway.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.
Assuming the prior section specifies when to deploy the bytecode, this bullet will become unnecessary
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.
As mentioned in other thread, I don't think it is important to specify the deployment. But I suppose the bullet is unnecessary because that is the semantics of an evm call anyways.
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 bullet is unnecessary, other than as clarification: any non-value call to account with non-existing code has no discernible effect on state.
If you prefix the bullet with
Note:
orClarification:
, it becomes apparent that this bullet does not intend to add any behavioural changes.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.
@holiman But shouldn't it then rather say:
BEACON_ROOTS_ADDRESS
, the call must succeed as with any other account without codeI find "fail" a bit misleading here.
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 agree: fail or succeed depends on your point of view. Whole bullet is moot and should not exist