-
Notifications
You must be signed in to change notification settings - Fork 42
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
[INT-104] feat: add WasmModuleRoot verification #37
[INT-104] feat: add WasmModuleRoot verification #37
Conversation
]; | ||
|
||
export const currentMainnetWasmModuleRootIndex = WASMModuleRoots.indexOf( | ||
'0x5eF0D09d1E6204141B4d37530808eD19f60FBa35', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we also use the wasmModuleRoot at src/createRollupPrepareConfig.ts, should we create a global constant file for this? @spsjvc
'0xf4389b835497a910d7ba3ebfb77aa93da985634f3c052de1290360635be40c4a', // consensus-v11 | ||
]; | ||
|
||
export const currentMainnetWasmModuleRootIndex = WASMModuleRoots.indexOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this hash correct? I don't see it in WASMModuleRoots, so currentMainnetWasmModuleRootIndex will always be -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it is v10 now, sometimes is not -1 is because we just schedule to upgrade but hasn't upgraded: https://github.com/OffchainLabs/nitro/blob/2142d5f4d2e3d967535da8edd0f071803caa0375/Dockerfile#L152
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the hash is not correct, ty, will change it
if (0 <= index) { | ||
console.log('The state transition function is standard version'); | ||
// check if the rollups' arbos version is latest or not | ||
if (index < currentMainnetWasmModuleRootIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following with the last comment, this will never be true since I think currentMainnetWasmModuleRootIndex will always be -1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes is not -1 is because we just schedule to upgrade but hasn't upgraded
let warningMessage: string = ''; | ||
|
||
// get the wasmModuleRoot of the given orbit chain | ||
const moduleRoot = (await orbitHandler.readContract( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add this check inside the rollup.ts/rollupHandler()
function, instead of creating a new handler for it. What do you think? For now we can run all verifications from verifyRollup, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering we can have a separate command to just verify the wasm module root, so I created a new handler here, do you think we don't need to have a separate one? (Because we can also use this to check if they upgrade to our latest arbos version in the future, this is the reason why I created currentMainnetWasmModuleRootIndex
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a clear reason to have it in a separate command for now. I could be wrong though. If we want to check if a team upgraded to the latest ArbOS version we can run the whole rollup verifier to see if the rest of the parameters are properly set too. In any case, this is not very important now, since there will be changes while we integrate the script in the Orbit SDK, so not 100% sure about this, haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I will add it to rollup.ts/rollupHandler()
first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jason! 🙏 Left a few comments.
console.log('The state transition function is standard version'); | ||
// check if the rollups' arbos version is latest or not | ||
if (index < currentMainnetWasmModuleRootIndex) { | ||
warningMessages.push('Arbos version is old'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add both module roots here? Something like "... Rollup wasmModuleRoot is ${moduleRoot}. Latest standard wasmModuleRoot is ${WASMModuleRoots[currentMainnetWasmModuleRootIndex]}."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Added a minor comment on the warningMessage to have some extra info, but that's all.
Thanks @Jason-W123 !
No description provided.