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: replace store_update_{time,height} by metadata update #981

Closed
wants to merge 6 commits into from

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Nov 23, 2023

Observe that update time and height are always read and manipulated
together. Replace pairs of methods for reading, updating and deleting
time and height with a single combined method for each operation.

Specifically, replace:
a) client_update_{time,height} by client_update_meta,
b) store_update_{time,height} by store_update_meta and
c) delete_update_{time,height} by delete_update_meta.

This allows better optimised implementations which, for example, keep
both pieces of information in a single map and perform a single lookup
to read or update them.

Furthermore, since the host timestamp and height come from the
implementation, remove those arguments from the store_update_meta
method with the implementation fetching the information by itself.

Closes: #973


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (45b1c97) 70.23% compared to head (c0127c9) 70.11%.

Files Patch % Lines
ibc-clients/ics07-tendermint/src/client_state.rs 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #981      +/-   ##
==========================================
- Coverage   70.23%   70.11%   -0.12%     
==========================================
  Files         177      177              
  Lines       17733    17676      -57     
==========================================
- Hits        12455    12394      -61     
- Misses       5278     5282       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mina86 mina86 closed this Dec 5, 2023
@mina86 mina86 deleted the f branch December 5, 2023 14:30
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.

feat: replace store_update_{time,height} by metadata update
1 participant