-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add key for storing counterparty next sequence send during WriteUpgradeOpen. #5460
Add key for storing counterparty next sequence send during WriteUpgradeOpen. #5460
Conversation
modules/core/24-host/packet_keys.go
Outdated
KeyPacketAckPrefix = "acks" | ||
KeyPacketReceiptPrefix = "receipts" | ||
KeyPruningSequence = "pruningSequence" | ||
KeyCounterpartyNextSeqSend = "counterpartyNextSequenceSend" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't particularly like the key name here, looking for better suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PruningSequenceEnd
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think KeyCounterpartyNextSeqSend
sounds good to me.
91b3201
to
658c298
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@@ -553,6 +553,16 @@ func (k Keeper) WriteUpgradeOpenChannel(ctx sdk.Context, portID, channelID strin | |||
k.SetNextSequenceAck(ctx, portID, channelID, upgrade.NextSequenceSend) | |||
} | |||
|
|||
// set the counterparty next sequence send in order to have upper bound to prune to | |||
k.SetNextSequenceSend(ctx, portID, channelID, counterpartyUpgrade.NextSequenceSend) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref aditya's comment in the other pr, the test can be updated to make sure these are set (pruning and next seq send)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be k.SetCounterpartyNextSequenceSend
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️ the perils of similarly named things
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## charly/set_nextseqrecv #5460 +/- ##
==========================================================
+ Coverage 80.77% 80.82% +0.05%
==========================================================
Files 197 197
Lines 15133 15159 +26
==========================================================
+ Hits 12223 12253 +30
+ Misses 2443 2438 -5
- Partials 467 468 +1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice and clean!
I'm unsure how I feel about the naming of the two different naming conventions for keys and getters/setters. Both sequences are used for pruning but they're naming doesn't lend them to be considered all that related imo!
The way I see it is:
PruningSequence
: The current sequence (lower bound) at which all previous acks and receipts have already been deleted.CounterpartyNextSequenceSend
: The current sequence (upper bound) for which it is safe to delete all previous acks and receipts.
// First upgrade for this channel will set the pruning sequence to 1, the starting sequence for pruning. | ||
// Subsequent upgrades will not modify the pruning sequence thereby allowing pruning to continue from the last | ||
// pruned sequence. | ||
if !k.HasPruningSequence(ctx, portID, channelID) { | ||
k.SetPruningSequence(ctx, portID, channelID, 1) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little bit unclear here as to when and where else the pruning sequence is mutated, exactly? Is it supposed to be in the pruning msg server handler?
its here, right? https://github.com/cosmos/ibc-go/pull/5444/files#diff-b225d8b78960f1ed19ccabcbdcda5ff708075790adac8463fbb0a83a8a63b78aR1145
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I would totally be up for this being set during first execution of a pruning message instead. The good thing of having it here is that we can see in 10 lines of code the initial bounds being set.
Following from the last comment on naming. We could also consider:
Any thoughts? cc. @colin-axner @charleenfei @chatton note: the field on the upgrade itself can remain as |
I'm pro the naming change @damiannolan suggests as well, these sequences are only stored after the upgrade completes for the sole purpose of pruning. So i don't think you have to keep the name the same as it is in the upgrade field. Store it under a pruning key that describes its particular purpose in the pruning logic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of PruningSequenceStart
& PruningSequenceEnd
suggested by @damiannolan to make things a little more explicit!
Otherwise everything else LGM
I am pro the naming changes as well as suggested by @damiannolan ~thanks! |
beautiful suggestion @damiannolan, very much in favor, |
a3d5f27
to
ff2b170
Compare
@@ -1476,32 +1476,53 @@ func (suite *KeeperTestSuite) TestWriteUpgradeOpenChannel_Ordering() { | |||
path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Ordering = types.UNORDERED | |||
}, | |||
preUpgrade: func() { | |||
ctx := suite.chainA.GetContext() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for additional noise, couldn't help myself.
989e0b1
to
7a14704
Compare
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
Description
to be merged after #5438
This value is then used as the upper bound for pruning packet acks/receipts. Also did a rename of the previous path/key to
PruningSequence
instead ofAcknowledgementPruningSequence
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.