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

[WIP] refactor: timestamps #53

Merged
merged 10 commits into from Jan 30, 2019
Merged

[WIP] refactor: timestamps #53

merged 10 commits into from Jan 30, 2019

Conversation

sleepdefic1t
Copy link
Contributor

@sleepdefic1t sleepdefic1t commented Jan 29, 2019

Proposed changes

Current Slots implementation sometimes overflows numeric timestamps on IoT.
Official Timestamps now also use milliseconds.

This PR does the following:

  • refactors and simplifies Slots.
  • remedies overflow/loss of precision of numeric timestamps.
  • fixes parsing of ISO8601 'Readable' Timestamps containing milliseconds.
  • updates the preset Ark Networks Timestamps to correctly use milliseconds.
  • sets a "dummy time" for PIO testing of IoT without a network connection.
  • updates Slots tests to reflect changes.
  • adds custom Slots::now() under USE_IOT flag.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • Test (adding missing tests or fixing existing tests)
  • Other... Please describe:

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Copy link
Contributor

@ciband ciband left a comment

Choose a reason for hiding this comment

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

The original slots structure and unit tests were designed to match core. I would vote to keep the interface as close to core as possible. What do the other APIs do?

@codecov-io
Copy link

Codecov Report

Merging #53 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   61.62%   61.71%   +0.08%     
==========================================
  Files          41       40       -1     
  Lines        2567     2565       -2     
==========================================
+ Hits         1582     1583       +1     
+ Misses        985      982       -3
Impacted Files Coverage Δ
src/include/cpp-crypto/networks/mainnet.h 100% <ø> (ø) ⬆️
src/include/cpp-crypto/networks/devnet.h 100% <ø> (ø) ⬆️
src/include/cpp-crypto/networks/testnet.h 100% <ø> (ø) ⬆️
src/utils/slot.cpp 91.66% <100%> (+7.05%) ⬆️
src/date/date.h 30.76% <0%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b607d93...02384ab. Read the comment docs.

@sleepdefic1t sleepdefic1t changed the title refactor: timestamps [WIP] refactor: timestamps Jan 29, 2019
@sleepdefic1t sleepdefic1t changed the title [WIP] refactor: timestamps refactor: timestamps Jan 30, 2019
@sleepdefic1t sleepdefic1t changed the title refactor: timestamps [WIP] refactor: timestamps Jan 30, 2019
@sleepdefic1t
Copy link
Contributor Author

@ciband

I saw they were going after matching Core.

Whatever was going on, it was overflowing timestamps on <=ESP32.
I was also unable to send transactions from the ESP32 using both Client & Crypto.

Timestamps also use milliseconds now https://explorer.ark.io:8443/api/v2/node/configuration.

After these most recent updates, I'm able to create and sign a transactions to be broadcasted via Cpp-Client:

https://dexplorer.ark.io/transaction/7745ade9c676fe7ebf9e114c61ff9065808413428f6f265da424eede55cb69de

Looks like PIO is having a fit about localTime on Linux PIO builds though.
On the fix for that.

@sleepdefic1t
Copy link
Contributor Author

I could work on using the naming in Core though.

It just needed a good housekeeping and was holding me back on IoT.

Was able to send another one after the last build.
https://dexplorer.ark.io/transaction/112cc3f11e5a318e3898075b9984a1398ae7e695811a31ec8d22069861e6bd36

And now there's a macOS build error 😭

@sleepdefic1t
Copy link
Contributor Author

sleepdefic1t commented Jan 30, 2019

@ciband
Correction, I don't think we're supposed to mimic Core's methods

SDK docs call for the following:

Slot

  • time Get the time elapsed since network start.
  • epoch Get the timestamp of the network start.

src: https://docs.ark.io/sdk/cryptography/guidelines.html#slot

@sleepdefic1t
Copy link
Contributor Author

@faustbrian Do you have a way to retrigger the workflow for this failing test?

I suspect it’s just a random CI error.
Slots do indirectly touch the tx.builder,
but that shouldn’t cause DER encoding to throw an assertion error.

I’d feel better seeing all tests pass, and this PR is otherwise complete.

Please and thank you.

@faustbrian faustbrian merged commit 7ba41df into ArkEcosystemArchive:master Jan 30, 2019
@sleepdefic1t sleepdefic1t mentioned this pull request Feb 5, 2019
12 tasks
@sleepdefic1t sleepdefic1t deleted the refactor/slots branch February 15, 2019 17:54
ciband pushed a commit to ciband/cpp-crypto that referenced this pull request Feb 18, 2019
* refactor: Slots

- add ms to Network Epochs
- refactor and fix Slot to parse Epoch ms
- update Slots tests
- update PIO 'temp_main.cpp' to set dummy board time

* fix: add IoT 'Slots::now'

* fix: add board specific code
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.

None yet

4 participants