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

evm: Add source chain topic to TransferRedeemed and MessageAttestedTo events #566

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

nvsriram
Copy link
Collaborator

@nvsriram nvsriram commented Dec 3, 2024

This PR adds:

  • sourceChain topic to MessageAttestedTo and TransferRedeemed events
  • Update event emit with propagated sourceChain
  • Update test and docs to account for new event signature

@nvsriram nvsriram marked this pull request as ready for review December 3, 2024 01:15
@nvsriram nvsriram force-pushed the evm/add-source-chain-to-redeem-events branch from 48060cd to ac18169 Compare December 3, 2024 01:16
@nvsriram nvsriram marked this pull request as draft December 3, 2024 01:17
@nvsriram nvsriram marked this pull request as ready for review December 3, 2024 01:41
@nvsriram nvsriram force-pushed the evm/add-source-chain-to-redeem-events branch from ac18169 to a95edc8 Compare December 3, 2024 18:06
@nik-suri nik-suri requested a review from djb15 December 3, 2024 18:19
Copy link
Collaborator

@djb15 djb15 left a comment

Choose a reason for hiding this comment

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

High level Q: what's the motivation for this change? Why do we need the source chain id in these events?

evm/src/NttManager/ManagerBase.sol Outdated Show resolved Hide resolved
evm/src/interfaces/IRateLimiter.sol Outdated Show resolved Hide resolved
@nvsriram
Copy link
Collaborator Author

nvsriram commented Dec 5, 2024

High level Q: what's the motivation for this change? Why do we need the source chain id in these events?

Main context behind the change is to address this issue: #504.

Copy link
Collaborator

@djb15 djb15 left a comment

Choose a reason for hiding this comment

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

Is there the intention for integrators to potentially upgrade existing deployments?

I'd love to see a couple of tests that involve having queued transfers (both inbound and outbound) with a previous deployment, before upgrading to this functionality and trying to release the queued transfers. You may have to do this by manually writing into a slot with the old struct format to fake a queued transfer before an upgrade; that way your test doesn't have to deal with upgrades directly.

For reference, this is why I suggested you move the the new fields to the end of the structs as otherwise the reads of existing queued transfers will be borked; moving to the end of the struct should work but a test to verify would be great.

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.

Emit source chain in the destination chain redeem/MsgAttestedTo events
2 participants