YAS-197 Require Message Bug

[ben-chain]

Description

This PR solves a more general case of our require message bug. It appears to have stemmed from constants being the last things in pre-transpiled code--constants sometimes had a PUSHN byte in them, followed by less than N bytes. This caused the constant to "eat into" the JUMP table.

Questions

I solved this by just looking for this case and padding if found. Is there a better way? We could put constant AFTER the JUMP table, but this seems like more complexity vs. just a padding.

Also, if this PUSH consumption ever occurs for some other reason than a constant that happens to have it, it could cause issues. It could be worth adding a warnings to transpilation which continues the transpilation but surfaces the warnings' visibility beyond a log.

Metadata

Fixes

• Fixes YAS-197

Contributing Agreement

• I have read and understood the Optimism Contributing Guide and Code of Conduct and am following those guidelines in this pull request.

[ben-chain]

Note -- this branches off of #26 , so easier to review that first.

[willmeister]

Looks like it works!

There's a lot of stuff that could be simplified or changed to follow best practices that will really help when we need to debug or extend this to do something else. Most comments are about that. Address whatever is reasonable, and get this merged in because there are a lot of dependencies on this.

[willmeister]

Big block comments like this are usually a good indication that this should be a separate function. Ideally complex functions read like a table of contents that gives a good gist of what is going on, with the in-depth logic for each part compartmentalized in its own little section (a new function) that can be viewed if wanting to dig deeper.

[willmeister]

Would re-write this function with 2 main focuses:

1. Replace big multi-line comments with intuitively-named helper functions
2. De-dupe the if/else

let indexOfCodeCopyOffset: number
let logPrefix: string
if (this.isNonLibraryPattern(bytecode, index)) {
  indexOfCodeCopyOffset = index + 2
  logPrefix = 'detected a NON-LIBRARY [CODECOPY(deployed bytecode)... RETURN] (CREATE/2 deployment logic)'
} else if (this.isLibraryPattern(bytecode, index)) {
  indexOfCodeCopyOffset = index + 1
  logPrefix = 'detected a LIBRARY [CODECOPY(deployed bytecode)... RETURN] (library deployment logic)'
}

if (!!indexOfCodeCopyOffset) {
  log.debug(`${logPrefix} pattern starting at PC: 0x${getPCOfEVMBytecodeIndex(index,bytecode).toString(16)}. Tagging the offset and size...`)
  bytecode[index] = {
    opcode: op.opcode,
    consumedBytes: op.consumedBytes,
    tag: {
      padPUSH: true,
      reasonTagged: IS_DEPLOY_CODE_LENGTH,
      metadata: undefined,
    },
  }
       
  bytecode[indexOfCodeCopyOffset] = {
    opcode: bytecode[indexOfCodeCopyOffset].opcode,
    consumedBytes: bytecode[indexOfCodeCopyOffset].consumedBytes,
    tag: {
      padPUSH: true,
      reasonTagged: IS_DEPLOY_CODECOPY_OFFSET,
      metadata: undefined,
    },
  }
}

return bytecode

[willmeister]

Doesn't matter, but just curious about the reason for this change.

[willmeister]

Suggested change

	log.debug(`Transpiled contract ${contractName}.`)
	log.debug(`Transpiled contract ${contractName}. Bytecode before library link replacement: ${JSON.stringify(output)}`)

[willmeister]

Multi-line comments like this should be a method-header in the helper function. The purpose of the helper function is to remove complexity from the flow of this function, but having many lines of reasoning means that the complexity has not been adequately removed.

[willmeister]

Don't worry about addressing this, though. As mentioned, will clean this up later.

[willmeister]

This function is not general purpose, so I'd just name it how it's being used so it is very clear what it does: getTranspiledLibraryPlaceholder(libraryName: string)

[willmeister]

Don't worry about this -- will update later.