Review and align details to current EOF spec

smlxl / evm.codes

Source for evm.codes an Ethereum Virtual Machine Opcodes Interactive Reference

https://evm.codes/

MIT License

718 stars 132 forks source link

Review and align details to current EOF spec #346

Closed pdobacz closed 3 days ago

pdobacz commented 1 week ago

First of all, thank you for adding EOF to evm.codes :bow:.

This is a first pass at fixing some details differing respect to current EOF spec, to my best recollection. Don't treat it as exhaustive yet.

I'm opening the PR as draft in order to get a round of reviews from the EOF team, in case I missed something or worded badly, but of course all feedback from maintainers is very welcome too :).

Here some comments on some of the changes:

I remove the conditions from "## Error cases" which are validated for EOF in deploy time and as such impossible to occur at runtime. The sections' wording appears to refer strictly to runtime
I suggest to add a section about instruction validation to each of the EOF instructions, but this is beyond the scope of the PR - requires discussion on how to approach this
I admit I haven't tested the .ts change, assuming it should "just work", if there's any doubts, lmk

vercel[bot] commented 1 week ago

@pdobacz is attempting to deploy a commit to the smlXL Team on Vercel.

A member of the Team first needs to authorize it.

pdobacz commented 1 week ago

@2xic @dorlevi tagging you since you've worked on the original EOF PR.

This is ready for your review.

vercel[bot] commented 1 week ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
evm-codes	✅ Ready (Inspect)	Visit Preview	Sep 16, 2024 8:04am

dorlevi commented 1 week ago

I authorized a deployment so it will create a preview in Vercel. super excited about your contributions we will review shortly cc: @2xic, apologies for the slight delay, we are super heads down on sim.io

2xic commented 3 days ago

Merge activity

Sep 16, 10:32 AM GMT+2: Graphite couldn't merge this PR because it failed for an unknown reason (Stack merges are not currently supported for forked repositories. Please create a branch in the target repository in order to merge).
Sep 16, 10:33 AM GMT+2: @2xic started a stack merge that includes this pull request via Graphite.
Sep 16, 10:33 AM GMT+2: Graphite couldn't merge this PR because it failed for an unknown reason (Stack merges are not currently supported for forked repositories. Please create a branch in the target repository in order to merge).

2xic commented 3 days ago

Thanks again @pdobacz @gumb0 @shemnon for the contribution 🙌