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

feat: adding EOFv1 opcodes #339

Closed
wants to merge 38 commits into from

Conversation

hangleang
Copy link
Contributor

@hangleang hangleang commented Jul 30, 2024

reference to issue #329 (comment)

Copy link

vercel bot commented Jul 30, 2024

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

A member of the Team first needs to authorize it.

@hangleang hangleang marked this pull request as draft July 30, 2024 10:37
Copy link

vercel bot commented Jul 30, 2024

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

Name Status Preview Updated (UTC)
evm-codes ✅ Ready (Inspect) Visit Preview Aug 30, 2024 1:00pm

@hangleang
Copy link
Contributor Author

once merge, I would like to close #329 as well

@hangleang hangleang marked this pull request as ready for review August 6, 2024 08:58
Copy link
Contributor

@charisra charisra left a comment

Choose a reason for hiding this comment

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

Few comments to start with

.replace('Gas', '')
.concat('Gas')
}
// @ts-ignore it's warn on ide, but technically it's work
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is correct? Also fix the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is warning because I try to install two different built (once on local submodule, once on remote registry), but I did link to different package names as in setup_monorepo.sh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the name's EIP param set has been changed on the EOF-supported version in ethjs: https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/evm/src/params.ts.

I also have been asked in their community, there is no built-in function to transform from pre-EOF and EOF-supported version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charisra could you confirm whether the answer satisfied your question or not?

@charisra
Copy link
Contributor

charisra commented Aug 6, 2024

@hangleang please add a PR description mentioning what this PR addresses and how we can test the functionality added. Also remove the WIP from the title, if you've finished the work.

@hangleang
Copy link
Contributor Author

hangleang commented Aug 20, 2024

@2xic I reverted your change on the setup_monorepo.sh script, since it will introduced internal conflicts in the monorepo itself

@2xic
Copy link
Collaborator

2xic commented Aug 21, 2024

@2xic I reverted your change on the setup_monorepo.sh script, since it will introduced internal conflicts in the monorepo itself

oh, yeah, good point. No worries about that then.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed internally just adding it as it's own fork (EOF) as things are in flux.

How do you feel about that ? This way we won't show cancun all over the place when it's technically a EOF fork.

Screenshot 2024-08-21 at 21 54 41

Copy link
Collaborator

@2xic 2xic left a comment

Choose a reason for hiding this comment

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

great 🎉 some minor comments, let me know if anything is unclear, but I think it's almost ready to be merged :shipit:

One other thing - is playground example something you want to add now that the playground is working, or is that something we need to add ?

@hangleang
Copy link
Contributor Author

Thank you, I believe it would be more sense if your team could add those examples, maybe update the doc reference as well. You understand the context and how those opcodes are stack together better than I do.

@2xic
Copy link
Collaborator

2xic commented Aug 22, 2024

Thank you, I believe it would be more sense if your team could add those examples, maybe update the doc reference as well. You understand the context and how those opcodes are stack together better than I do.

Alright, I'll check that up 🫡

@hangleang
Copy link
Contributor Author

hey @2xic, as your suggestion, I removed the EOF toggle, and add prague fork with EOF opcodes

@2xic
Copy link
Collaborator

2xic commented Aug 26, 2024

hey @2xic, as your suggestion, I removed the EOF toggle, and add prague fork with EOF opcodes

Awesome @hangleang , two minor things (I hope)

  1. can we call it just EOF instead of Prague ?

We had some discussion internally and since things are still a bit in flux about whether it will be in Prague or not, we feel it better to just call it EOF)

  1. Can we still have the default fork be Cancun as that is the current active fork ?

Let me know if anything is unclear / it's something we need to do ourselves.

@hangleang
Copy link
Contributor Author

hangleang commented Aug 26, 2024

  1. I override the prague hardfork with those EOF opcodes, I was trying to use customHardfork prop, but didn't work. could you point me to that custom configs?
    custom_hardfork

@2xic
Copy link
Collaborator

2xic commented Aug 28, 2024

  1. I override the prague hardfork with those EOF opcodes, I was trying to use customHardfork prop, but didn't work. could you point me to that custom configs?
    custom_hardfork

True, so I think we can just add some fallback (show a different fork in the UI, but just still have EOFCommon use the Prague fork as that is what EthereumJS uses)

Here is some example diff that seems to work (we probably need to update all the docs that references prague and just change it to EOF which should be simple, I updated just two of them to test )

diff --git a/components/ChainSelector.tsx b/components/ChainSelector.tsx
index f7d967f..f2c76f8 100644
--- a/components/ChainSelector.tsx
+++ b/components/ChainSelector.tsx
@@ -6,21 +6,19 @@ import Select, { OnChangeValue, components } from 'react-select'
 
 import { EthereumContext } from 'context/ethereumContext'
 
-import { CURRENT_FORK, EOF_ENABLED_FORK } from 'util/constants'
+import { CURRENT_FORK } from 'util/constants'
 import { toKeyIndex } from 'util/string'
 
 import { Icon, Label } from 'components/ui'
 
 const ChainOption = (props: any) => {
-  const { data, children } = props
+  const { data, label } = props
   const isCurrent = data.value === CURRENT_FORK
-  const isEOFEnabled = data.value === EOF_ENABLED_FORK
 
   return (
     <components.Option {...props}>
-      {children}
+      {label}
       {isCurrent && <Label>Live</Label>}
-      {isEOFEnabled && <Label>EOF</Label>}
     </components.Option>
   )
 }
@@ -60,10 +58,15 @@ const ChainSelector = () => {
     }
 
     if (!router.query.fork) {
-      const latestFork = forks.at(-1)
-      const fork = forkOptions.find((fork) => fork.value === latestFork?.name)
+      const fork = forkOptions.find((fork) => fork.value === CURRENT_FORK)
       setForkValue(fork as any)
       onForkChange(fork?.value as string)
+    } else {
+      const fork = forkOptions.find((fork) => fork.value === router.query.fork)
+      if (fork) {
+        setForkValue(fork as any)
+        onForkChange(fork?.value as string)
+      }
     }
 
     // eslint-disable-next-line react-hooks/exhaustive-deps
diff --git a/context/ethereumContext.tsx b/context/ethereumContext.tsx
index 572401a..e726ae0 100644
--- a/context/ethereumContext.tsx
+++ b/context/ethereumContext.tsx
@@ -37,6 +37,7 @@ import {
 import {
   CURRENT_FORK,
   EOF_ENABLED_FORK,
+  EOF_FORK_NAME,
   FORKS_WITH_TIMESTAMPS,
 } from 'util/constants'
 import {
@@ -182,10 +183,11 @@ export const EthereumProvider: React.FC<{}> = ({ children }) => {
     chainId?: Chain,
     fork?: string,
   ) => {
+    const forName = fork == EOF_FORK_NAME ? EOF_ENABLED_FORK : fork
     common = new EOFCommon({
       chain: Chain.Mainnet,
-      hardfork: fork || CURRENT_FORK,
-      eips: fork == EOF_ENABLED_FORK ? EOF_EIPS : [],
+      hardfork: forName || CURRENT_FORK,
+      eips: forName == EOF_ENABLED_FORK ? EOF_EIPS : [],
     })
 
     vm = await EOFVM.create({ common })
@@ -488,6 +490,11 @@ export const EthereumProvider: React.FC<{}> = ({ children }) => {
       }
     })
 
+    forks.push({
+      name: EOF_FORK_NAME,
+      block: null,
+    })
+
     setForks(forks)
   }
 
diff --git a/docs/opcodes/E0.mdx b/docs/opcodes/E0.mdx
index 8834dcc..71cc7cd 100644
--- a/docs/opcodes/E0.mdx
+++ b/docs/opcodes/E0.mdx
@@ -1,5 +1,5 @@
 ---
-fork: Prague
+fork: EOF
 group: Stack Memory Storage and Flow Operations
 ---
 
diff --git a/docs/opcodes/EE.mdx b/docs/opcodes/EE.mdx
index ad89755..a7651c7 100644
--- a/docs/opcodes/EE.mdx
+++ b/docs/opcodes/EE.mdx
@@ -1,5 +1,5 @@
 ---
-fork: Prague
+fork: EOF
 group: System operations
 ---
 
diff --git a/util/constants.ts b/util/constants.ts
index c75762e..4810963 100644
--- a/util/constants.ts
+++ b/util/constants.ts
@@ -4,9 +4,9 @@ export const GITHUB_REPO_URL = 'https://github.com/smlxl/evm.codes'
 // See: https://github.com/ethereumjs/ethereumjs-monorepo/tree/master/packages/common/src/hardforks
 export const CURRENT_FORK = 'cancun'
 export const EOF_ENABLED_FORK = 'prague'
+export const EOF_FORK_NAME = 'EOF'
 
 export const FORKS_WITH_TIMESTAMPS: { [name: string]: number } = {
   shanghai: 1681338455,
   cancun: 1710338135,
-  prague: 1710338135,
 }

@2xic
Copy link
Collaborator

2xic commented Aug 30, 2024

@hangleang Can you apply the diff below diff also ? I noticed dynamic fees got hidden because we changed the name (and also that sometimes the editor would act weird when loading a fork, so updated the loading init code)

diff --git a/context/ethereumContext.tsx b/context/ethereumContext.tsx
index 9596285..4c9dc62 100644
--- a/context/ethereumContext.tsx
+++ b/context/ethereumContext.tsx
@@ -171,18 +171,19 @@ export const EthereumProvider: React.FC<{}> = ({ children }) => {
   const breakpointIds = useRef<number[]>([])
 
   useEffect(() => {
-    initVmInstance()
     // eslint-disable-next-line react-hooks/exhaustive-deps
+    _loadChainAndForks(
+      new EOFCommon({
+        chain: Chain.Mainnet,
+        hardfork: EOF_ENABLED_FORK,
+      }),
+    )
   }, [])
 
   /**
    * Initializes the EVM instance.
    */
-  const initVmInstance = async (
-    skipChainsLoading?: boolean,
-    chainId?: Chain,
-    fork?: string,
-  ) => {
+  const initVmInstance = async (fork?: string) => {
     const forkName = fork == EOF_FORK_NAME ? EOF_ENABLED_FORK : fork
     common = new EOFCommon({
       chain: Chain.Mainnet,
@@ -198,10 +199,6 @@ export const EthereumProvider: React.FC<{}> = ({ children }) => {
 
     currentOpcodes = evm.getActiveOpcodes()
 
-    if (!skipChainsLoading) {
-      _loadChainAndForks(common)
-    }
-
     _loadOpcodes()
     _loadPrecompiled()
     _setupStateManager()
@@ -224,7 +221,7 @@ export const EthereumProvider: React.FC<{}> = ({ children }) => {
     if (chain) {
       setSelectedChain(chain)
       resetExecution()
-      initVmInstance(true, chainId, selectedFork?.name)
+      initVmInstance(selectedFork?.name)
     }
   }
 
@@ -237,7 +234,7 @@ export const EthereumProvider: React.FC<{}> = ({ children }) => {
     if (fork) {
       setSelectedFork(fork)
       resetExecution()
-      initVmInstance(true, selectedChain?.id, forkName)
+      initVmInstance(forkName)
     }
   }
 
diff --git a/opcodes.json b/opcodes.json
index 338bba8..bac0333 100644
--- a/opcodes.json
+++ b/opcodes.json
@@ -1456,7 +1456,7 @@
     "output": "",
     "description": "Copy a segment of data section to the stack",
     "dynamicFee": {
-      "prague": {
+      "EOF": {
         "inputs": {
           "offset": {
             "type": "number",
@@ -1599,7 +1599,7 @@
     "output": "address",
     "description": "Create a contract using EOF container at given index",
     "dynamicFee": {
-      "prague": {
+      "EOF": {
         "inputs": {
           "offset": {
             "type": "number",
@@ -1639,7 +1639,7 @@
     "output": "",
     "description": "Deploy container to an address",
     "dynamicFee": {
-      "prague": {
+      "EOF": {
         "inputs": {
           "auxDataOffset": {
             "type": "number",
@@ -2023,7 +2023,7 @@
     "output": "status",
     "description": "Drop-in replacement for CALL instruction",
     "dynamicFee": {
-      "prague": {
+      "EOF": {
         "inputs": {
           "value": {
             "type": "number",
@@ -2062,7 +2062,7 @@
     "output": "status",
     "description": "Drop-in replacement for DELEGATECALL instruction",
     "dynamicFee": {
-      "prague": {
+      "EOF": {
         "inputs": {
           "offset": {
             "type": "number",
@@ -2160,7 +2160,7 @@
     "output": "status",
     "description": "Drop-in replacement for STATICCALL instruction",
     "dynamicFee": {
-      "prague": {
+      "EOF": {
         "inputs": {
           "offset": {
             "type": "number",

@hangleang
Copy link
Contributor Author

@2xic thank for correct me, it seems work as expected, but don't forget to double check since I'm not following the whole diff.

I still keep initVmInstance when page loaded, just apply block number for the EOF fork. also I renamed all prague.mdx files to EOF.mdx to show gas formula on dynamic fee

@2xic
Copy link
Collaborator

2xic commented Aug 30, 2024

@2xic thank for correct me, it seems work as expected, but don't forget to double check since I'm not following the whole diff.

I still keep initVmInstance when page loaded, just apply block number for the EOF fork. also I renamed all prague.mdx files to EOF.mdx to show gas formula on dynamic fee

Awesome - that also seemed to have done the trick. Thank you!

@2xic 2xic mentioned this pull request Aug 30, 2024
@2xic
Copy link
Collaborator

2xic commented Aug 30, 2024

Thanks @hangleang . There was some changes we added in #342, but built it all on top of your commits. Thanks so much for your contribution!

@2xic 2xic closed this Aug 30, 2024
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.

6 participants