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

Enforce consistent datetime serialization #3389

Merged
merged 16 commits into from
Jul 5, 2024

Conversation

sug0
Copy link
Contributor

@sug0 sug0 commented Jun 7, 2024

Describe your changes

Our DateTimeUtc type allowed a relaxed representation of RFC3339 strings. We now enforce a string subset of this format, to guarantee deterministic serialization.

Indicate on which release or other PRs this topic is based on

v0.39.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@sug0 sug0 added the bug Something isn't working label Jun 7, 2024
sug0 added a commit that referenced this pull request Jun 7, 2024
@sug0 sug0 requested a review from tzemanovic June 7, 2024 12:44
sug0 added a commit that referenced this pull request Jun 7, 2024
@sug0 sug0 force-pushed the tiago/consistent-datetime-serialization branch from a587e9b to 1ef2adf Compare June 7, 2024 12:48
tzemanovic
tzemanovic previously approved these changes Jun 7, 2024
Copy link
Member

@tzemanovic tzemanovic 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, just in crates/apps_lib/src/cli.rs pls update help for NAMADA_START_TIME

sug0 added a commit that referenced this pull request Jun 7, 2024
@sug0 sug0 force-pushed the tiago/consistent-datetime-serialization branch from 1ef2adf to 7f38267 Compare June 7, 2024 15:07
sug0 added a commit that referenced this pull request Jun 7, 2024
@sug0 sug0 force-pushed the tiago/consistent-datetime-serialization branch from 7f38267 to 3352219 Compare June 7, 2024 15:15
sug0 added a commit that referenced this pull request Jun 7, 2024
@sug0 sug0 force-pushed the tiago/consistent-datetime-serialization branch from 3352219 to 834a468 Compare June 7, 2024 15:29
@sug0 sug0 requested a review from tzemanovic June 7, 2024 15:29
Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 91.89189% with 6 lines in your changes missing coverage. Please review.

Project coverage is 53.93%. Comparing base (879a326) to head (7f90393).
Report is 4 commits behind head on main.

Files Patch % Lines
crates/apps_lib/src/cli.rs 0.00% 4 Missing ⚠️
crates/core/src/time.rs 95.83% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3389   +/-   ##
=======================================
  Coverage   53.92%   53.93%           
=======================================
  Files         317      317           
  Lines      107575   107610   +35     
=======================================
+ Hits        58011    58037   +26     
- Misses      49564    49573    +9     

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

@cwgoes cwgoes mentioned this pull request Jun 7, 2024
@sug0 sug0 marked this pull request as draft June 7, 2024 20:35
sug0 added a commit that referenced this pull request Jun 7, 2024
@sug0 sug0 force-pushed the tiago/consistent-datetime-serialization branch from 834a468 to c18864e Compare June 7, 2024 21:10
@sug0 sug0 marked this pull request as ready for review June 7, 2024 21:11
@sug0 sug0 requested a review from murisi June 7, 2024 21:12
murisi
murisi previously approved these changes Jun 8, 2024
Copy link
Contributor

@murisi murisi 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 to me. Thanks!

@murisi murisi dismissed their stale review June 8, 2024 07:21

Test completeness.

Copy link
Contributor

@murisi murisi left a comment

Choose a reason for hiding this comment

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

Just to be sure that hardware wallet will continue to work, could we please demonstrate in a unit test that DateTimeUtc is a subset of DateTime somewhere?

crates/core/src/time.rs Show resolved Hide resolved
crates/core/src/time.rs Outdated Show resolved Hide resolved
brentstone added a commit that referenced this pull request Jun 12, 2024
* tiago/consistent-datetime-serialization:
  Changelog for #3389
  Improve tx salting
  Fix from tm time impl for `DateTimeUtc`
  Misc fixes
  Increase gas limit in `FinalizeBlock` tests
  Rebuild tx fixtures
  Resign localnet genesis txs
  Rebuild wasms for tests
  gen_localnet.py: Fix genesis time string
  Add datetime encoding tests
  Enforce fixed RFC3339 format
@sug0 sug0 force-pushed the tiago/consistent-datetime-serialization branch from 0ac361d to 7f90393 Compare June 18, 2024 09:55
@murisi
Copy link
Contributor

murisi commented Jun 18, 2024

@murisi could you please test this pr with the ledger hw wallet?

This PR works with the hardware wallet. We just need to make a (hopefully) minor change on the hardware wallet to increase the precision it displays. See for instance the following where Actual is hardware wallet output and Expected is what's produced in the test vectors.

1: Expected: is equal to "2 | Timestamp : 4900-04-22 06:26:45.041754692 UTC"
1:   Actual: "2 | Timestamp : 4900-04-22 06:26:45.041754 UTC"

murisi
murisi previously approved these changes Jun 18, 2024
Copy link
Contributor

@murisi murisi 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 to me. Thanks!

@murisi murisi dismissed their stale review June 18, 2024 14:33

Test vectors contain data not found in serializations.

murisi
murisi previously requested changes Jun 18, 2024
Copy link
Contributor

@murisi murisi left a comment

Choose a reason for hiding this comment

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

Could we please change the line

format!("Timestamp : {}", tx.header.timestamp.0),
to format!("Timestamp : {}", tx.header.timestamp), ? This will change the expected output on the Ledger app from 2 | Timestamp : 4900-04-22 06:26:45.041754692 UTC to something like 3 | Timestamp : 4440-02-19T23:31:34.674481+00:00. This is required because the Borsh serialization does not contain the last 3 decimal places.

Either we do the above, or we increase the number of decimal places in the Borsh serialization of a DateTimeUtc so that the hardware wallet is able to meet the precise expectations.

@murisi murisi dismissed their stale review June 18, 2024 15:58

Tested changes before the forced pushes.

Copy link
Contributor

@murisi murisi left a comment

Choose a reason for hiding this comment

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

Great! This PR does not break the hardware wallet. Thanks!

brentstone added a commit that referenced this pull request Jun 25, 2024
* tiago/consistent-datetime-serialization:
  Changelog for #3389
  Regen tx fixtures
  Resign localnet genesis txs
  Rebuild wasms for tests
  gen_localnet.py: Fix genesis time string
  Increase precision of timestamps to 9 nanos
  Fix genesis time in tests
  Keep nanoseconds during CometBFT time conversions
  Switch to fixed offset format in UTC
  Increase robustness of datetime test
  Improve tx salting
  Fix from tm time impl for `DateTimeUtc`
  Misc fixes
  Increase gas limit in `FinalizeBlock` tests
  Add datetime encoding tests
  Enforce fixed RFC3339 format
brentstone added a commit that referenced this pull request Jun 25, 2024
* tiago/consistent-datetime-serialization:
  Changelog for #3389
  Regen tx fixtures
  Resign localnet genesis txs
  Rebuild wasms for tests
  gen_localnet.py: Fix genesis time string
  Increase precision of timestamps to 9 nanos
  Fix genesis time in tests
  Keep nanoseconds during CometBFT time conversions
  Switch to fixed offset format in UTC
  Increase robustness of datetime test
  Improve tx salting
  Fix from tm time impl for `DateTimeUtc`
  Misc fixes
  Increase gas limit in `FinalizeBlock` tests
  Add datetime encoding tests
  Enforce fixed RFC3339 format
brentstone added a commit that referenced this pull request Jul 4, 2024
* origin/tiago/consistent-datetime-serialization:
  Changelog for #3389
  Regen tx fixtures
  Resign localnet genesis txs
  Rebuild wasms for tests
  gen_localnet.py: Fix genesis time string
  Increase precision of timestamps to 9 nanos
  Fix genesis time in tests
  Keep nanoseconds during CometBFT time conversions
  Switch to fixed offset format in UTC
  Increase robustness of datetime test
  Improve tx salting
  Fix from tm time impl for `DateTimeUtc`
  Misc fixes
  Increase gas limit in `FinalizeBlock` tests
  Add datetime encoding tests
  Enforce fixed RFC3339 format
@brentstone brentstone merged commit b9ce576 into main Jul 5, 2024
19 of 21 checks passed
@brentstone brentstone deleted the tiago/consistent-datetime-serialization branch July 5, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants