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

fix(events): align event processing with smart contract changes #400

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

ezreal1997
Copy link
Contributor

align event processing with smart contract changes

issue: closes #393

Copy link
Contributor

@edisonz0718 edisonz0718 left a comment

Choose a reason for hiding this comment

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

I think we need to make this backward compatible since it's tricky to upgrade CL and contract together.

@jdubpark
Copy link
Contributor

jdubpark commented Dec 4, 2024

I think we need to make this backward compatible since it's tricky to upgrade CL and contract together.

I agree, it requires precise timing of smart contract upgrade to follow the updated CL.

Alternatively we could do this:

  1. Update the smart contract to emit two logs in each operator function, ie. emit AddOperator & SetOperator and RemoveOperator & UnsetOperator.
  2. Update the CL code.
  3. Nodes that have not upgraded will pick old logs (Add/Remove) and nodes that have upgraded will pick new logs (Set/Unset).
  4. At some point, update the smart contract to remove the AddOperator and RemoveOperator event logs. (This is the backward-incompatible step but we have both CL and contracts ready)

@ezreal1997
Copy link
Contributor Author

ezreal1997 commented Dec 5, 2024

I think we need to make this backward compatible since it's tricky to upgrade CL and contract together.

Anyway, we need to notify the community that we will deprecate addOperator and removeOperator func, so I prefer a simpler way:

  1. Include this change in the next hardfork.
  2. Then we notify the community 1) old contract funcs to be deprecated and occurrence time is the next hardfork time, 2) new contract funcs to be supported and the occurrence time is xxx blocks/hours right after hardfork.

cc @edisonz0718 @jdubpark

@edisonz0718
Copy link
Contributor

I think we need to make this backward compatible since it's tricky to upgrade CL and contract together.

Anyway, we need to notify the community that we will deprecate addOperator and removeOperator func, so I prefer a simpler way:

  1. Include this change in the next hardfork.
  2. Then we notify the community 1) old contract funcs to be deprecated and occurrence time is the next hardfork time, 2) new contract funcs to be supported and the occurrence time is xxx blocks/hours right after hardfork.

cc @edisonz0718 @jdubpark

This will cause a service disruption if someone wants to use operator functions between CL hardfork and contract upgrade. I think a simpler approach is to just make CL backward compatible. Add the new operator functions instead of replacing. So that both types of event can be processed.

@ezreal1997
Copy link
Contributor Author

I think we need to make this backward compatible since it's tricky to upgrade CL and contract together.

Anyway, we need to notify the community that we will deprecate addOperator and removeOperator func, so I prefer a simpler way:

  1. Include this change in the next hardfork.
  2. Then we notify the community 1) old contract funcs to be deprecated and occurrence time is the next hardfork time, 2) new contract funcs to be supported and the occurrence time is xxx blocks/hours right after hardfork.

cc @edisonz0718 @jdubpark

This will cause a service disruption if someone wants to use operator functions between CL hardfork and contract upgrade. I think a simpler approach is to just make CL backward compatible. Add the new operator functions instead of replacing. So that both types of event can be processed.

If we plan to keep both operator functions until Odyssey deprecated, that makes sense. @Ramarti plz add the old operator func back.

@0xHansLee
Copy link
Contributor

Yeah, I also agree to @edisonz0718. We can make CL backward compatible for the contract upgrade for now, and deprecate the old events after upgrading the contract.

@ezreal1997
Copy link
Contributor Author

My point is if we do not keep old operator functions, it's already a non-compatible change on user side, so it's equivalent if CL is backward compatible or not.

If we want to make backward compatible, we need following steps:

  1. upgrade CL to handler both old and new events
  2. upgrade contract to keep both old and new operator functions
  3. ... one month for eco projects to make the change ...
  4. upgrade the contract to remove the old operator functions
  5. upgrade CL to remove the processing logic of old events

@limengformal
Copy link
Contributor

As discussed with @edisonz0718 , we will not fix #393 in odyssey since it is a name change. We will apply the fix in mainnet version.

@edisonz0718 edisonz0718 merged commit a1a917e into operator_methods Dec 12, 2024
1 check passed
@edisonz0718 edisonz0718 deleted the rayden/align-event-process branch December 12, 2024 18:17
Ramarti pushed a commit that referenced this pull request Dec 13, 2024
align event processing with smart contract changes

issue: closes #393
Ramarti pushed a commit that referenced this pull request Dec 13, 2024
align event processing with smart contract changes

issue: closes #393
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.

5 participants