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: [EXC-1675] migrate replica to no-op LogVisibilityV2 #768

Merged
merged 30 commits into from
Aug 9, 2024

Conversation

maksymar
Copy link
Contributor

@maksymar maksymar commented Aug 6, 2024

This PR migrates replica to using a no-op LogVisibilityV2 instead of LogVisibility.

The difference between the two is in AllowedViewers enum variant which is currently ignored, so it's expected for V2 to behave the same way as V1.

@maksymar maksymar changed the title log_visibility_v2 feat: migrate replica to LogVisibilityV2 Aug 6, 2024
@github-actions github-actions bot added the feat label Aug 6, 2024
@maksymar maksymar changed the title feat: migrate replica to LogVisibilityV2 feat: [EXC-1675] migrate replica to LogVisibilityV2 Aug 7, 2024
@maksymar maksymar changed the title feat: [EXC-1675] migrate replica to LogVisibilityV2 feat: [EXC-1675] migrate replica to no-op LogVisibilityV2 Aug 7, 2024
@maksymar
Copy link
Contributor Author

maksymar commented Aug 8, 2024

Note: do not submit before updating nns/sns proto messages.

UPD: it was decided not to do any nns/sns proto message changes.

@maksymar maksymar marked this pull request as ready for review August 8, 2024 07:31
@maksymar maksymar requested review from a team as code owners August 8, 2024 07:31
@dsarlis
Copy link
Member

dsarlis commented Aug 8, 2024

Note: do not submit before updating nns/sns proto messages.

I think we can let the NNS team take care of updating their interface if they are interested to use the new log visibility and focus only on updating the replica to use the new type (canisters can still send the old type and we know what to do with it). There's no reason to add a dependency in our process on the NNS/SNS canisters to be upgraded before we can proceed with further steps.

@maksymar
Copy link
Contributor Author

maksymar commented Aug 8, 2024

Note: do not submit before updating nns/sns proto messages.

I think we can let the NNS team take care of updating their interface if they are interested to use the new log visibility and focus only on updating the replica to use the new type (canisters can still send the old type and we know what to do with it). There's no reason to add a dependency in our process on the NNS/SNS canisters to be upgraded before we can proceed with further steps.

I had to update rs/nns/cmc/cmc.did with new type so that their test passes and after that I suppose we have to update nns proto.

@dsarlis
Copy link
Member

dsarlis commented Aug 8, 2024

Note: do not submit before updating nns/sns proto messages.

I think we can let the NNS team take care of updating their interface if they are interested to use the new log visibility and focus only on updating the replica to use the new type (canisters can still send the old type and we know what to do with it). There's no reason to add a dependency in our process on the NNS/SNS canisters to be upgraded before we can proceed with further steps.

I had to update rs/nns/cmc/cmc.did with new type so that their test passes and after that I suppose we have to update nns proto.

What if you don't make the change in their tests to use V2? I doubt that you'd need to change any interface in that case. We can leave this update to after the replica fully uses V2 (the only thing blocked is completely removing V1 which is something that can wait a bit longer).

EDIT: Wait I see it now. This direct dependency on management types is killing us. We need to fix it. I'll try to push for a resolution because we've had similar issues in the past.

rs/nns/cmc/cmc.did Outdated Show resolved Hide resolved
Copy link
Contributor

@aterga aterga left a comment

Choose a reason for hiding this comment

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

LGTM modulo CMC field optionality.

@maksymar
Copy link
Contributor Author

maksymar commented Aug 8, 2024

LGTM modulo CMC field optionality.

I removed cmc.did change, and added a copy-type to overcome type dependency from management canister. PTAL.

@maksymar maksymar requested a review from dsarlis August 9, 2024 12:03
rs/nns/cmc/src/lib.rs Outdated Show resolved Hide resolved
@maksymar maksymar enabled auto-merge August 9, 2024 14:09
@maksymar maksymar added this pull request to the merge queue Aug 9, 2024
Merged via the queue into master with commit f040350 Aug 9, 2024
23 checks passed
@maksymar maksymar deleted the maksym/exc-1670-v2 branch August 9, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants