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: remote omni-chain call context: sender and chainID #912

Merged
merged 26 commits into from
Aug 9, 2023

Conversation

brewmaster012
Copy link
Collaborator

@brewmaster012 brewmaster012 commented Aug 4, 2023

Description

NOTE to a3 network:
Will require a redeployment of SystemContract and a MsgUpdateSystemContractAddress from policy group admin,
post upgrade (supposed in V8.0.0)

  1. This PR uses the latest SystemContract from zeta-chain/protocol-contracts@bbfea49
    which adds the context struct to the parameters of depositAndCall() function in SystemContract.

This is breaking for dApps that rely on omnichain smart contract--the interface has changed.
image

  1. This PR introduces new admin type Msg to update system contract address to a new address.
    The Msg handler updates all references to systemcontract address in ZRC20 contracts tracked
    by ForeignCoins, and also migrate state from old systemcontract to new systemcontract.

Added three smoketest:

  1. Revised all zevm swap app to use the new interface
  2. Added a dummy omnichain smart contract that emits the context to verify the context is correct.
  3. tested the Upgrade SystemContract address at block 20.

Misc fixes:

  1. fixed smoketest bitcoin tx occasional signing fail
  2. fixed some CI scripts

Closes: #832
Closes: #918
Closes: #919

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

@brewmaster012
Copy link
Collaborator Author

@fadeev This PR will require a doc update.

@brewmaster012 brewmaster012 changed the title WIP: Remote omni-chain call context: sender and chainID feat: remote omni-chain call context: sender and chainID Aug 7, 2023
x/fungible/keeper/deposits.go Show resolved Hide resolved
x/fungible/keeper/deposits.go Outdated Show resolved Hide resolved
x/fungible/keeper/msg_server_update_system_contract.go Outdated Show resolved Hide resolved
x/fungible/module.go Outdated Show resolved Hide resolved
x/fungible/module.go Outdated Show resolved Hide resolved
x/fungible/types/message_update_system_contract.go Outdated Show resolved Hide resolved
@brewmaster012
Copy link
Collaborator Author

@lumtis thank you for the comments; addressed most of them. Request re-review

@lumtis
Copy link
Member

lumtis commented Aug 7, 2023

@lumtis thank you for the comments; addressed most of them. Request re-review

Great, I will do a second iteration of the PR tomorrow, need to take more time looking at the smoke tests

@lumtis
Copy link
Member

lumtis commented Aug 8, 2023

@brewmaster012 it seems there might be some comments you haven't seen, any opinion on these suggestions?

@fadeev
Copy link
Member

fadeev commented Aug 8, 2023

@brewmaster012 can you provide more info on the origin and the sender values? As far as I can tell sender is the address that sends tokens to a TSS address, what is the origin then?

@brewmaster012
Copy link
Collaborator Author

@brewmaster012 can you provide more info on the origin and the sender values? As far as I can tell sender is the address that sends tokens to a TSS address, what is the origin then?

Right now, origin is the EOA address, sender is empty because we are only processing txs that sent to TSS directly from EOA, not from some contract.

@brewmaster012
Copy link
Collaborator Author

@brewmaster012 it seems there might be some comments you haven't seen, any opinion on these suggestions?

yeah missed them. Now all should be addressed.

@lumtis
Copy link
Member

lumtis commented Aug 8, 2023

@brewmaster012 it seems there might be some comments you haven't seen, any opinion on these suggestions?

yeah missed them. Now all should be addressed.

Thanks. It seems merging 0.46.13 created many errors in the CI

Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

It looks legit to me, just another comment.

@brewmaster012
Copy link
Collaborator Author

brewmaster012 commented Aug 8, 2023 via email

which renames Context => zContext
@brewmaster012
Copy link
Collaborator Author

ready for approval. @lumtis @kingpinXD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants