Skip to content
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

Added EIP-2330: EXTSLOAD #2330

Merged
merged 3 commits into from
Nov 1, 2019
Merged

Added EIP-2330: EXTSLOAD #2330

merged 3 commits into from
Nov 1, 2019

Conversation

dominicletz
Copy link
Contributor

Initial draft submission, cross-posting on ethereum-magicians

EIPS/eip-2121.md Outdated Show resolved Hide resolved
EIPS/eip-2121.md Outdated
SLOAD (0x5c)
```

* A "fixed(@N)" solidity keyword that marks a contract storage variable as externally readable. The @N is a storage slot address and forces the compiler to put the contract variable at the specified slot N. The data position is thus becoming part of the interface.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Solidity syntax should be discussed on https://github.com/ethereum/solidity and EIPs changing the EVM have little say in the syntax. They can suggest things, but not as part of a specification.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally there is a very lengthy discussion on the topic of selecting storage slots in the language: ethereum/solidity#597

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think you're suggesting to split this issue into two -- One for the EVM opcode and one for the Solidity syntax?

Thanks for the link to the discussion. I was not aware of it. The reasoning is very different though. ethereum/solidity#597 is about contract upgrades while this is about supporting off-chain applications (and off-chain evm less applications). I'll try to make my case in a new solidity issue over there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@axic is there something like a solidity EIP process or should I just open an issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no such thing as a Solidity EIP process. Design discussion about the language are not handling in this repo. I'd suggest to open an issue or continue discussion under ethereum/solidity#597.

It is perfectly fine giving examples or explaining what Solidity should do as part of this EIP, but a Solidity specification doesn't belong here. Additionally restricting this EIP to EVM changes only makes the scope smaller, easier to understand and reason about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning is very different though. ethereum/solidity#597 is about contract upgrades while this is about supporting off-chain applications (and off-chain evm less applications).

The reason is different, but there are a bunch of issues on the topic of setting fixed storage locations in the language.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, will split and open a solidity issue

EIPS/eip-2330.md Outdated Show resolved Hide resolved
@dominicletz dominicletz changed the title Added EIP-2121: SLOAD2 and ABI Added EIP-2330: SLOAD2 and ABI Oct 29, 2019
@dominicletz dominicletz changed the title Added EIP-2330: SLOAD2 and ABI Added EIP-2330: SLOAD2 Oct 29, 2019
@dominicletz
Copy link
Contributor Author

@axic thanks alot for your comments. I'm heading out now for dinner, but will post a solidity issue tomorrow.

@dominicletz dominicletz changed the title Added EIP-2330: SLOAD2 Added EIP-2330: EXTSLOAD Nov 1, 2019
EIPS/eip-2330.md Outdated
@@ -1,8 +1,8 @@
---
eip: 2330
title: SLOAD2
title: EXTSLOAD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you want to extend the title a bit like "EXTSLOAD opcode".

@axic
Copy link
Member

axic commented Nov 1, 2019

I'd also suggest to add a link to the Solidity issue to the example part to aid discussion.

EIPS/eip-2330.md Outdated
A new EVM instruction `EXTSLOAD (0x5c)` that works like `SLOAD (0x54)` with the gas cost of EXTCODE(700) + SLOAD(800) = 1500 and an additional parameter representing the contract that is to be read from.

```
EXTSLOAD <contract> <slot> (0x5c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this specification is a bit confusing. Is it having two immediate parameters? Is it taking those from the stack?

Please check out https://eips.ethereum.org/EIPS/eip-1052 and https://eips.ethereum.org/EIPS/eip-145 as examples on the specification section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've looked at those and (hopefully) clarified the specification accordingly.

Copy link
Member

@axic axic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is well written for a draft.

@axic axic merged commit ee0bf86 into ethereum:master Nov 1, 2019
ilanolkies pushed a commit to ilanolkies/EIPs that referenced this pull request Nov 12, 2019
* Added EIP-2330: SLOAD2 and ABI

* Renamed SLOAD2 to EXTSLOAD

* Clarifying opcode specification
MadeofTin pushed a commit to MadeofTin/EIPs that referenced this pull request Nov 13, 2019
* Added EIP-2330: SLOAD2 and ABI

* Renamed SLOAD2 to EXTSLOAD

* Clarifying opcode specification
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
* Added EIP-2330: SLOAD2 and ABI

* Renamed SLOAD2 to EXTSLOAD

* Clarifying opcode specification
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
* Added EIP-2330: SLOAD2 and ABI

* Renamed SLOAD2 to EXTSLOAD

* Clarifying opcode specification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants