feat: add sender when calling connected chain contracts from zetachain #350

skosito · 2024-09-12T14:37:17Z

converting to ready for review, without generated files to make review easier

on zevm side, in call method added CallContext including old field gasLimit and new field isArbitraryCall which is emitted in Called event (note that msg.sender is already emitted) - this is added for withdrawAndCall methods as well

^ all these methods are overloaded to keep compatibility with current protocol offchain implementation, but imo it introduces not needed overhead - should we just keep them until next release? or just use 1 method only now, this will be used on localnet only anyways

on evm side, add MessageContext for execute method, containing sender and isArbitraryCall so execute method can decide to call any sc, or sc with onCall implemented passing sender to it (currently task suggests whole MessageContext, but just sender is needed, interface can be simplified, not sure what origin and chainID should be?)

questions:

zevm side, assume we need CallContext on withdrawAndCall methods?
evm side, do we want MessageContext on executeWithERC20/revertWithERC20 and executeRevert functions?

coderabbitai · 2024-09-12T14:37:25Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Base branches to auto review (1)

main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
-- I pushed a fix in commit <commit_id>, please review it.
-- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
-- @coderabbitai generate unit testing code for this file.
-- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
-- @coderabbitai generate interesting stats about this repository and render them as a table.
-- @coderabbitai read src/utils.ts and generate unit testing code.
-- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
-- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

v2/contracts/evm/GatewayEVM.sol

codecov-commenter · 2024-09-12T20:25:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.55%. Comparing base (bef2648) to head (7319423).

Additional details and impacted files

@@                      Coverage Diff                      @@
##           rename-on-crosschain-call     #350      +/-   ##
=============================================================
+ Coverage                      97.37%   97.55%   +0.17%     
=============================================================
  Files                              7        7              
  Lines                            305      327      +22     
  Branches                          98      105       +7     
=============================================================
+ Hits                             297      319      +22     
  Misses                             8        8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lumtis

Looks good to me generally on the tests

Will need @fadeev review for the interface

v2/contracts/evm/GatewayEVM.sol

lumtis · 2024-09-16T19:13:13Z

v2/contracts/evm/interfaces/IGatewayEVM.sol

+
+/// @notice Interface implemented by contracts receiving authenticated calls.
+interface Callable {
+    function onCall(address sender, bytes calldata message) external returns (bytes memory);


Can we maybe pass the entire context object to keep some consistency with the version on ZetaChain?

yea but currently messageContext contains sender and flag to indicate if its arb or auth call, so passing that flag here doesnt make much sense

let's see if we need to extend this context further, maybe we can define new context struct for this, waiting from @fadeev for more input on interface, but can easily be extended

maybe this is bad design to have this flag in messageContext, execute method that contains message context is immediately authenticated call, method without context is arbitrary call

lets then see what to put in message context beside sender and i will change it

fixed, lets see what else to put in message context apart from sender

lumtis · 2024-09-19T10:02:02Z

@skosito I think we could already prepare the change in the protocol before merging this one: zeta-chain/node#2877

fadeev

Looks good!

skosito · 2024-09-24T15:49:17Z

closing for this one, it is the the same, but base branch is correct #350

skosito added 3 commits September 12, 2024 16:24

min changes for new functionality

bc7b177

Merge branch 'rename-on-crosschain-call' into on-call-connected-chains

48d8d83

simplify interface

9a81d8a

lumtis reviewed Sep 12, 2024

View reviewed changes

v2/contracts/evm/GatewayEVM.sol Outdated Show resolved Hide resolved

skosito added 3 commits September 12, 2024 22:14

fixes so tests pass

5ac4847

prevent calling onCall in execute arb call

b40d43a

refactor a bit

27e58e7

skosito added 2 commits September 12, 2024 23:26

add overloads for withdrawAndcall functions

d05288d

missing natspec

ec645b2

skosito changed the title ~~On call connected chains~~ feat: add sender when calling connected chain contracts from zetachain Sep 13, 2024

skosito added 2 commits September 13, 2024 14:41

add execute with msg context tests

d245ed1

tests

40385f9

skosito linked an issue Sep 13, 2024 that may be closed by this pull request

Enable calling contracts on connected chains with caller info #344

Closed

skosito marked this pull request as ready for review September 13, 2024 13:52

skosito requested review from andresaiello, brewmaster012, CharlieMc0, fadeev and fbac as code owners September 13, 2024 13:52

skosito requested a review from lumtis September 13, 2024 15:41

lumtis reviewed Sep 16, 2024

View reviewed changes

skosito added 2 commits September 16, 2024 22:50

pr comment

df3576c

refactor execute funcs

7319423

fadeev approved these changes Sep 23, 2024

View reviewed changes

skosito closed this Sep 24, 2024

skosito mentioned this pull request Sep 24, 2024

feat: add sender when calling connected chain contracts from zetachain #355

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add sender when calling connected chain contracts from zetachain #350

feat: add sender when calling connected chain contracts from zetachain #350

skosito commented Sep 12, 2024 •

edited

Loading

coderabbitai bot commented Sep 12, 2024 •

edited

Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

codecov-commenter commented Sep 12, 2024 •

edited

Loading

lumtis left a comment

lumtis Sep 16, 2024

skosito Sep 16, 2024

skosito Sep 16, 2024

skosito Sep 16, 2024

lumtis commented Sep 19, 2024 •

edited

Loading

fadeev left a comment

skosito commented Sep 24, 2024

feat: add sender when calling connected chain contracts from zetachain #350

feat: add sender when calling connected chain contracts from zetachain #350

Conversation

skosito commented Sep 12, 2024 • edited Loading

coderabbitai bot commented Sep 12, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

Documentation and Community

codecov-commenter commented Sep 12, 2024 • edited Loading

Codecov Report

lumtis left a comment

Choose a reason for hiding this comment

lumtis Sep 16, 2024

Choose a reason for hiding this comment

skosito Sep 16, 2024

Choose a reason for hiding this comment

skosito Sep 16, 2024

Choose a reason for hiding this comment

skosito Sep 16, 2024

Choose a reason for hiding this comment

lumtis commented Sep 19, 2024 • edited Loading

fadeev left a comment

Choose a reason for hiding this comment

skosito commented Sep 24, 2024

skosito commented Sep 12, 2024 •

edited

Loading

coderabbitai bot commented Sep 12, 2024 •

edited

Loading

codecov-commenter commented Sep 12, 2024 •

edited

Loading

lumtis commented Sep 19, 2024 •

edited

Loading