-
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 pruning logic, implementation in message server. #5444
Merged
crodriguezvega
merged 23 commits into
04-channel-upgrades
from
jim/5414-msg-server-keeper-prune-acks
Dec 21, 2023
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
036b0b0
Impl msg server, keeper logic.
DimitrisJim 07df54f
Update naming used for start/end as per updates for keys.
DimitrisJim 8787fdf
Self contained implementation logic in keeper.
DimitrisJim 6b5ada0
Lint, test structure, error case checking.
DimitrisJim 0c7d8e4
Test test teeeeest.
DimitrisJim cafb419
Rename keeper function, document a couple of additional things.
DimitrisJim e78cfd4
Test case: send packets after upgrade, no pruning.
DimitrisJim 83ed801
Return number pruned, number left in response.
DimitrisJim a7d62dc
Update modules/core/04-channel/keeper/keeper_test.go
DimitrisJim d53d659
expSequenceStart -> expPruningSequenceStart
DimitrisJim 84bb6a2
make: proto-format
DimitrisJim 80a0440
rename per Aditya's suggestion (PruneStalePacketState)
DimitrisJim 84a3b99
rename MsgPruneAcknowledgements to MsgPruneStalePacketState
crodriguezvega a63b27d
Revert "rename MsgPruneAcknowledgements to MsgPruneStalePacketState"
crodriguezvega 089afaa
rename MsgPruneAcknowledgements to MsgPruneStalePacketData
crodriguezvega 3a5a2ef
Merge branch '04-channel-upgrades' into jim/5414-msg-server-keeper-pr…
crodriguezvega dca600c
pr review
charleenfei 3bdc4d6
chore: add total prefixes to response args and cleanup keeper funcs
damiannolan 998e7ba
chore: fixing linter issues
chatton 530c8ad
re-re-name back to PruneAcknowledgements
crodriguezvega 90eda36
Merge branch '04-channel-upgrades' into jim/5414-msg-server-keeper-pr…
crodriguezvega bcf8535
remove calls to Has functions
crodriguezvega 9b36dc8
Update modules/core/04-channel/keeper/keeper.go
crodriguezvega File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 find it odd that the get func here doesn't return a
found
bool. But I guess it is fineThere 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 agree, I found I had the same reaction too!
But apparently the argument is saving gas with a
Has
check? :D lolThere 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.
Isn't this increasing gas? I suspect the majority of calls to this function to have the sequence set, which results in extra gas costs
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 it would increase gas in the successful case of it being set. And save gas in the scenario of it not being set
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.
Yea! Looking at the gas config earlier though, this should be miniscule enough to not warrant it, we can totally switch it up to have get return val, found as usual, my bad in adding it this way.