eof: Pass _eofVersion down to libevmasm/Assembly class

[rodiazet]

No description provided.

[github-actions]

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

[ekpyron]

Looking at it again in isolation I do have a few more, but only minor comments - I've not yet seen why this would make that one test failing that CI complains about (I had assumed the CommonOptions::get().eofVersion() would also be empty for now?).

[ekpyron]

I actually didn't mean just passing std::nullopt here, but not adding the eof version option to Compiler (and CompilerContext) in the first place (and just have them pass std::nullopt to the Assemblys they need and such)
And an assert

solUnimplementedAssert(!m_eofVersion, "EOF is not supported for legacy code generation");

here also wouldn't hurt, together with the !m_viaIR assert below maybe.

[ekpyron]

Also looking a bit closer here: should a sub-assembly get its own EOF version? I'd imagine that it'd always inherit the EOF version from the parent Assembly. (In the EthAssemblyAdapter implementation you can fetch it from the wrapped Assembly, in NoOutputAssembly, you'd need to store it.)

Or is the idea that technically there could be EOF version 1 containers containing EOF version 2 containers?

[ekpyron]

Suggested change

	bool const eof = m_eofVersion.has_value();
	bool const eof = m_eofVersion.has_value();
	solAssert(!eof || m_eofVersion == 1, "Invalid EOF version.");

[rodiazet]

Done

[rodiazet]

Looking at it again in isolation I do have a few more, but only minor comments - I've not yet seen why this would make that one test failing that CI complains about (I had assumed the CommonOptions::get().eofVersion() would also be empty for now?).

Problem is in https://github.com/ipsilon/solidity/blob/eof-prep-1/test/libsolidity/Metadata.cpp#L228 test. It doesn't make sense for now. Even changing first evm version to prague() does not help because the codegen produce a code with PushTag which is not allowed for eof and is asserts in https://github.com/ipsilon/solidity/blob/eof-prep-1/libevmasm/Assembly.cpp#L965.

The test should be added later and adjusted to new EOF version. I can move it to proper commit (later PR) of the eof-cleaned branch. Let me know your opinion.

[ekpyron]

Looking at it again in isolation I do have a few more, but only minor comments - I've not yet seen why this would make that one test failing that CI complains about (I had assumed the CommonOptions::get().eofVersion() would also be empty for now?).

Problem is in https://github.com/ipsilon/solidity/blob/eof-prep-1/test/libsolidity/Metadata.cpp#L228 test. It doesn't make sense for now. Even changing first evm version to prague() does not help because the codegen produce a code with PushTag which is not allowed for eof and is asserts in https://github.com/ipsilon/solidity/blob/eof-prep-1/libevmasm/Assembly.cpp#L965.

The test should be added later and adjusted to new EOF version. I can move it to proper commit (later PR) of the eof-cleaned branch. Let me know your opinion.

Makes sense, yeah - we need clean CI to merge this, but we can just remove the test for now, merge this PR and bring the test back later, yes!