Skip to content
This repository has been archived by the owner on Jan 11, 2024. It is now read-only.

Remove check #266

Merged
merged 2 commits into from
Nov 2, 2023
Merged

Remove check #266

merged 2 commits into from
Nov 2, 2023

Conversation

cryptoAtwill
Copy link
Collaborator

Remove the check on checkpointInfo.threshold due to:

  • 0 threshold is also valid threshold.
  • Checkpoint info is created together with checkpoint, as long as checkpoint is created, checkpoint info will be created. Checking checkpoint should be enough.

@@ -231,9 +231,6 @@ contract GatewayRouterFacet is GatewayActorModifiers {
}

CheckpointInfo storage checkpointInfo = s.bottomUpCheckpointInfo[height];
if (checkpointInfo.threshold == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The goal of this check was to verify that a checkpoint had been created. Before that, we suggested that the threshold cannot be equal to 0; in that case, equality to 0 meant that the checkpoint did not exist.
If the threshold can be equal to 0, then you need to implement another mechanism that allows you to verify the existence of checkpoint and checkpoint information.

@@ -307,9 +304,6 @@ contract GatewayRouterFacet is GatewayActorModifiers {
if (s.bottomUpCheckpoints[checkpoint.blockHeight].blockHeight > 0) {
revert CheckpointAlreadyExists();
}
if (s.bottomUpCheckpointInfo[checkpoint.blockHeight].threshold > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The same comment from above is applied here.
You cannot guarantee that separate data types are managed synchronously. We need a mechanism to validate that.

You can combine those types or add a new field signaling whether checkpoint/info exists or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so effectively, we can create some kind of lib or wrapper to manage these two together? Because what happens now is the existence of checkpoint implies the existence of checkpoint info.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we need an additional library for that.
And I do not see any problems with your last statement either.

It is enough to add a bool field in both structs and check its value instead of checking threshold

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true though that at the moment both of them are accessed together every time: inserted, read and deleted in unison. A wrapper with 2 fields (checkpoint and info) could save an extra dictionary traversal.

Copy link
Contributor

@adlrocha adlrocha left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. The code looks cleaner to me know. I would let @dnkolegov, have a final pass before merging as I know he had some concerns (that being said looks fine for me).

@cryptoAtwill cryptoAtwill merged commit d08fc57 into dev Nov 2, 2023
3 of 6 checks passed
@cryptoAtwill cryptoAtwill deleted the align-weight branch November 2, 2023 03:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants