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

add timestamp to ethr builder #48

Merged
merged 11 commits into from
Feb 15, 2024
Merged

add timestamp to ethr builder #48

merged 11 commits into from
Feb 15, 2024

Conversation

insipx
Copy link
Contributor

@insipx insipx commented Feb 14, 2024

related to xmtp/xps-gateway#61

timestamp is not the best way b/c we can have multiple installation id updates in the same block. It may be OK in the general case, since it could be rare that a user updates their installation key in the same block twice, but it can happen. libxmtp relies on it in order to sort through installation ids it already has. In the future it would be better to use a block # / log index combo. This would also avoid yet another api call to the ethereum rpc node

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

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

Comparison is base (c7f7452) 97.27% compared to head (77f5c5a) 97.25%.

Files Patch % Lines
lib/src/types/ethr.rs 97.97% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
- Coverage   97.27%   97.25%   -0.02%     
==========================================
  Files          13       13              
  Lines        2826     2953     +127     
==========================================
+ Hits         2749     2872     +123     
- Misses         77       81       +4     

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

@insipx insipx requested review from tsachiherman, jac18281828 and 37ng and removed request for tsachiherman February 14, 2024 18:19
Copy link
Contributor

@37ng 37ng left a comment

Choose a reason for hiding this comment

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

LGTM!
Looks like we need a rustfmt.toml to unify cargo fmt

@insipx
Copy link
Contributor Author

insipx commented Feb 14, 2024

LGTM! Looks like we need a rustfmt.toml to unify cargo fmt

we should be just on the stable default rustfmt settings, in this case my lsp crashed and i didn't get the autoformatting I usually do

Copy link
Contributor

@jac18281828 jac18281828 left a comment

Choose a reason for hiding this comment

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

I think we should be consistent in our use of block timestamp in seconds

lib/src/resolver.rs Outdated Show resolved Hide resolved
lib/src/resolver.rs Outdated Show resolved Hide resolved
@insipx
Copy link
Contributor Author

insipx commented Feb 14, 2024

removed logic to convert to datetime b/c it was wasteful, changed to store the usual timestamp and then multiply for nanoseconds @jac18281828

Copy link
Contributor

@jac18281828 jac18281828 left a comment

Choose a reason for hiding this comment

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

Looks good ! One last thing, I’d use Duration for time conversion.

lib/src/types/xmtp.rs Outdated Show resolved Hide resolved
@insipx insipx merged commit bb2c7d6 into main Feb 15, 2024
4 of 6 checks passed
@insipx insipx deleted the insipx/xps-timestamp branch February 15, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants