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: Deduplicate localize method for Step.sol #12

Conversation

pcw109550
Copy link
Contributor

@pcw109550 pcw109550 commented Feb 8, 2024

Description

Upstream cannon directly embeds PreimageKeyLib methods to its step contract. By including library functions(which only holds internal methods), bytecodes of localize method will be stored in step contract and called by JUMP instruction.

In asterisc's case, this change is nontrivial because entire Step method is implemented in yul. Previous PRs relied on duplication of implementation inside of STF code.

I found a workaround; define wrapper method which calls localize method from the library. The compiler thinks this method is not used by anywhere when it is not declared as public method, so explicitly mark as public. After that, delegatecall the method, to preserve msg.sender.

However after finding this approach, It feels like it is overly complicated. Should we allow duplications and copy-paste localize implementation, by closing this PR?

Is there any better method while deduplication?

Tests

EVM mode tests will cover this diff.

Additional context

Current dependencies on/for this PR:

@pcw109550 pcw109550 force-pushed the tip/pcw109550/fix-preimage-oracle-off-by-one branch from 83cb772 to 6a384a8 Compare February 8, 2024 05:47
@pcw109550 pcw109550 force-pushed the tip/pcw109550/deduplicate-localize branch from f6b2a85 to 6a0fc01 Compare February 8, 2024 05:47
@pcw109550 pcw109550 force-pushed the tip/pcw109550/fix-preimage-oracle-off-by-one branch 2 times, most recently from e7da1f6 to 6b0f2ee Compare February 8, 2024 06:01
@pcw109550 pcw109550 force-pushed the tip/pcw109550/deduplicate-localize branch from 6a0fc01 to bbcd6c0 Compare February 8, 2024 06:03
@pcw109550 pcw109550 force-pushed the tip/pcw109550/fix-preimage-oracle-off-by-one branch from 6b0f2ee to 0b86f77 Compare February 8, 2024 06:19
@pcw109550 pcw109550 force-pushed the tip/pcw109550/deduplicate-localize branch from bbcd6c0 to 3f7fcd6 Compare February 8, 2024 06:19
@pcw109550 pcw109550 marked this pull request as ready for review February 8, 2024 09:08
@pcw109550 pcw109550 self-assigned this Feb 8, 2024
@ImTei
Copy link
Collaborator

ImTei commented Feb 13, 2024

I think the problem is that the gas cost of delegateCall is a bit expensive and someone can feel confused when reading the code. It may be worth If the duplicated code is more complicated or written in solidity, but it isn't. so I think it's ok to reuse same code for localize(). wdyt?

@pcw109550 pcw109550 changed the base branch from tip/pcw109550/fix-preimage-oracle-off-by-one to master February 13, 2024 09:16
This intentionally hints foundry to compile PreimageOracle.sol required for rvgo tests
@pcw109550 pcw109550 force-pushed the tip/pcw109550/deduplicate-localize branch from 3f7fcd6 to 2d09372 Compare February 14, 2024 07:21
@pcw109550 pcw109550 changed the base branch from master to tip/pcw109550/fetch-monorepo-contracts February 14, 2024 07:21
@pcw109550 pcw109550 force-pushed the tip/pcw109550/fetch-monorepo-contracts branch from 953dbcb to d706fbf Compare February 14, 2024 07:30
@clabby
Copy link
Member

clabby commented Feb 14, 2024

Should we allow duplications and copy-paste localize implementation, by closing this PR?

Yes, this is fine. The Preimage localization isn't too complex, just a few lines of code - I'd opt for duplicating the code rather than adding complexity here.

@pcw109550
Copy link
Contributor Author

Thanks for everyone's feedback: Agree with all of you and will close this.

@pcw109550 pcw109550 closed this Feb 14, 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.

None yet

3 participants