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

stdlib prototype #10639

Closed
wants to merge 18 commits into from
Closed

stdlib prototype #10639

wants to merge 18 commits into from

Conversation

axic
Copy link
Member

@axic axic commented Dec 16, 2020

Part of #10282.

TODO:

  • provide a way to export the standard library as a directory hierarchy from the commandline interface (or standard-json - then it would be a json map of filename -> string)
  • only include standard library files in the compiler stack input files if they were imported (use the import file resolution mechanism for that)
  • if a file is included whose name clashes with a file in the standard library (or maybe even the prefix std/), check if its contents match the built-in contents and don't do anything. Otherwise warn.
  • Changelog entry

If the three points are combined, you can export the standard library to a folder and importing from that folder would do the same as importing the built-in standard library.

The contents of the standard library are of course version-dependent.

assembly {
// allocates a memory block
// TODO: use memoryguard
function alloc(size) -> ptr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function alloc(size) -> ptr {
function allocate(size) -> ptr {

Copy link
Member Author

Choose a reason for hiding this comment

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

I think actually these memory functions may be better as being part of the inline assembly dialect (as discussed on #10399).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but I think it would be good to also have the translation between the two dialects available.

@@ -0,0 +1,53 @@
// SPDX-License-Identifier: Apache-2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why apache?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be MIT too. The goal is to avoid license baggage for users. They may want to keep their deployed codebase with less restrictions than GPL-3.0.

Copy link
Member

Choose a reason for hiding this comment

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

One thing I really don't like about Apache license is this bit:

You must cause any modified files to carry prominent notices stating that You changed the files

I.e. it requires the source to be annotated with extensive info about who changed it. I think that such metadata belongs in the version control, not in the file. It would be pretty tedious to have to keep it in the source.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it just means stating it was changed and not what has been. Though in practice this does not take place in the original source repo, just in forks.

Also note Rust uses a dual MIT/Apache2 license and a large part of the Ethereum ecosystem uses Apache2.

assembly {
// Prepare input
// We are using the free memory pointer.
let input := mload(0x40)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this into a function?

Copy link
Member Author

Choose a reason for hiding this comment

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

So far these files were independently working, and the assembly file of course does not work yet. But yes, eventually it should use the assembly helpers.

mstore(output, 0)

// Call the precompile
let ret := staticcall(gas(), 1, input, 128, output, 32)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't ecrecover need more error checks?

Copy link
Member Author

@axic axic Dec 21, 2020

Choose a reason for hiding this comment

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

This is currently a 1-on-1 mapping to ecrecover in the language. If we do semantic changes, then we can consider an empty output as failure and revert.

mstore(input, hash)
mstore(add(input, 32), v)
mstore(add(input, 64), r)
mstore(add(input, 96), s)
Copy link
Member Author

@axic axic Dec 21, 2020

Choose a reason for hiding this comment

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

Could also use bytes manipulation (as per discussions in #9829):

bytes input = new bytes(96);
input[0..32] = bytes32(bytes8(v));
input[32..64] = r;
input[64..96] = s;

Or

bytes input = abi.encodePacked(bytes32(bytes8(v)), r, s);

If we manage to improve memory handling 😉

@ethereum ethereum deleted a comment from stackenbotten Apr 22, 2021
@axic
Copy link
Member Author

axic commented Apr 23, 2021

I wonder if we should just start committing stdlib under semanticTests/externalSources and iterating on it? And once it is in a useful state, git move those files to /stdlib and get it exposed in the compiler.

Comment on lines 3 to 7
function blockhash(uint blockNumber) returns (bytes32 ret) {
assembly {
ret := blockhash(blockNumber)
}
}
Copy link
Member

@cameel cameel Apr 24, 2021

Choose a reason for hiding this comment

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

Just a heads up before we drop the source in the repo - I think that the piece of code as prominent as the standard library should really follow our own coding style :)

Suggested change
function blockhash(uint blockNumber) returns (bytes32 ret) {
assembly {
ret := blockhash(blockNumber)
}
}
function blockhash(uint blockNumber) returns (bytes32 ret) {
assembly {
ret := blockhash(blockNumber)
}
}

EDIT: turns out the _ prefix for parameters is not in our Solidity style guidelines. So only indents are wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Do we have any good guide for variable names (i.e. for ret)?

stdlib/utils.sol Outdated
Comment on lines 3 to 13
function min<T>(T a, T b) pure internal returns (T ret) {
ret = (a < b) ? a : b;
}

function max<T>(T a, T b) pure internal returns (T ret) {
ret = (a > b) ? a : b;
}

function abs<T: T.isSigned>(T a) pure internal returns (T ret) {
ret = (a < 0) ? -a : a;
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the future :)

//import sha256 from std;
//import chainid from std;

contract C{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
contract C{

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, this was before free functions :)

}
}

library Block {
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of libraries, we can split these into different files and import them based on file names.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think we should split this in many small files.

Copy link
Contributor

Choose a reason for hiding this comment

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

(mainly to test out if the way we plan to do namespacing in the future works out well or not)

uint constant PanicResourceError = 0x41;
uint constant PanicInvalidInternalFunction = 0x51;

error Panic(uint code);
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we natspec the stdlib?

@leonardoalt
Copy link
Member

@axic wen stdlib

@axic
Copy link
Member Author

axic commented Jun 27, 2022

Pushed here a new approach, where a compiler flag enables the stdlib. This way perhaps we can merge these 3 precompiles as a start.

@axic axic requested review from chriseth, cameel and leonardoalt June 27, 2022 07:54
CMakeLists.txt Show resolved Hide resolved
{

static std::map<std::string, std::string> sources = {
{ "std/precompiles.sol", precompiles },
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't find a good cmake way to generate this file 👁️

function sha256(bytes memory input) returns (bytes32 ret) {
assembly {
let success := staticcall(gas(), 2, add(input, 32), mload(input), 0, 32)
if iszero(success) { revert(0, 0) }
Copy link
Member Author

@axic axic Jun 27, 2022

Choose a reason for hiding this comment

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

These precompiles currently differ in two ways from the IR:

  1. only support newer EVM versions where passing all gas is possible (would need Introduce solidity.evmVersion() to query the target EVM #10283 to support conditional options)
  2. it does not pass through the revert data (this should be fixed before merging)

@ethereum ethereum deleted a comment from stackenbotten Jun 27, 2022
@ethereum ethereum deleted a comment from stackenbotten Jun 27, 2022
@ethereum ethereum deleted a comment from stackenbotten Jun 27, 2022
@ethereum ethereum deleted a comment from stackenbotten Jun 27, 2022
@github-actions
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 15, 2023
@ekpyron ekpyron removed the stale The issue/PR was marked as stale because it has been open for too long. label Mar 15, 2023
@ekpyron
Copy link
Member

ekpyron commented Mar 15, 2023

Removing the stale label here - but @nikola-matic, you wanted to check and report the state of this, s.t. we can decide all remaining open questions, if any (I guess it's mainly stuff like the name used in importing and such?), and actually move this forward, right?

[For the record: this only occurs in the roadmap as a "future" roadmap item - but that is supposed to mean to be finished with this - this PR is only the very first small step towards moving everything that can be moved and the entire thing will both take a lot of time and require generics - but that doesn't mean that we shouldn't still start and do what we can do "now" or at least soonish - no immediate hurry, but we also shouldn't just keep this open and go to waste forever]

@github-actions
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 30, 2023
@nikola-matic nikola-matic removed the stale The issue/PR was marked as stale because it has been open for too long. label Mar 30, 2023
@nikola-matic
Copy link
Collaborator

Removing the stale label here - but @nikola-matic, you wanted to check and report the state of this, s.t. we can decide all remaining open questions, if any (I guess it's mainly stuff like the name used in importing and such?), and actually move this forward, right?

[For the record: this only occurs in the roadmap as a "future" roadmap item - but that is supposed to mean to be finished with this - this PR is only the very first small step towards moving everything that can be moved and the entire thing will both take a lot of time and require generics - but that doesn't mean that we shouldn't still start and do what we can do "now" or at least soonish - no immediate hurry, but we also shouldn't just keep this open and go to waste forever]

We can add the roadmap label to this PR, so it doesn't get the bot's attention, right @r0qs?

@r0qs
Copy link
Member

r0qs commented Mar 30, 2023

Yes, or even use must have or must have eventually with prevent the bot to mark it as stale.
Just a small correction since must have is only used for issues and not PRs. So just use roadmap as you said.

@r0qs r0qs added the roadmap label Mar 30, 2023
@cameel
Copy link
Member

cameel commented Apr 19, 2023

Decision from the call: we'd prefer a special syntax for the stdlib imports:

import std.cryprography;

@nikola-matic
Copy link
Collaborator

nikola-matic commented Apr 19, 2023

Decision from the call: we'd prefer a special syntax for the stdlib imports:

import std.cryprography;

Changes to Parser::parseImportDirective() and the grammar then. Would be nice to crosspost this to the stdlib issue. Might be a bit annoying (haven't looked into it yet) to protect against allowing this syntax without the pragma being enabled.

@cameel
Copy link
Member

cameel commented Apr 19, 2023

I did cross-post there already. Small changes in the parser will of course be needed (the stdlib import will use an identifier path) but this will all be behind an experimental pragma so non-breaking.

And I don't think we'll be keeping the ANTLR grammar up to date with the experimental changes. @ekpyron already thinks that even trying to keep it up to date as is is too much of a distraction.

@nikola-matic
Copy link
Collaborator

I did cross-post there already. Small changes in the parser will of course be needed (the stdlib import will use an identifier path) but this will all be behind an experimental pragma so non-breaking.

And I don't think we'll be keeping the ANTLR grammar up to date with the experimental changes. @ekpyron already thinks that even trying to keep it up to date as is is too much of a distraction.

Ah, so we're changin the pragma directive as well. Should this be implemented separately as a prerequisite for stdlib, and then this PR updated respectively?

@cameel
Copy link
Member

cameel commented Apr 19, 2023

I think separately, because we want it done and merged in the very near future. It should not be held back by discussions of other changes in this PR.

@cameel
Copy link
Member

cameel commented Apr 19, 2023

@ekpyron said on the call that anyone not busy with other tasks could start working on that already. We'll discuss further steps next week.

@ekpyron
Copy link
Member

ekpyron commented Apr 19, 2023

Yeah, we won't maintain an antlr grammar for the experimental version of the language - we can do that once we have stabilized it :-).

And yes, this PR will be rebased on top of a generic "pragma experimental solidity-next;" PR, which we'll have soon (after that we can drop the pragma stdlib here) - and that experimental pragma will work in such a way that it can enable an experimental parser mode that can then be permissive in the import directive and allow this new style.

@ekpyron said on the call that anyone not busy with other tasks could start working on that already. We'll discuss further steps next week.

"That" being the pragma experimental solidity-next; PR first - and once that's done rebasing this PR on top of that, if there's even more time to spare.

@ekpyron
Copy link
Member

ekpyron commented May 30, 2023

Closing in favour of #14249

@ekpyron ekpyron closed this May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
roadmap selected for development It's on our short-term development
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants