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.

Sign up for GitHub

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

Jump to bottom

fix(exp): document expirationTimestamp #631

Merged
merged 5 commits into from Aug 15, 2022
Merged

fix(exp): document expirationTimestamp #631

merged 5 commits into from Aug 15, 2022

Conversation

whitneypurdum
Copy link
Member

@whitneypurdum whitneypurdum commented Aug 9, 2022 •
edited

Description

Updates documentation for expirationTimestamp to be explicit that the timestamp is in milliseconds

Contributor checklist

  • Breaking changes - check for any existing interfaces changes that are not backward compatible, removed method etc.
  • Documentation - document your code, add comments for method, remember to check if auto generated docs were updated.
  • Tests - add new or updated existed test for changes you made.
  • Migration guide - add migration guide for every breaking change.
  • Configuration correctness - check that any configuration changes are correct ex. default URLs, chain ids, smart contract verification on Volta explorer or EWC explorer.

jrhender
jrhender requested changes Aug 10, 2022
View reviewed changes
Copy link
Collaborator

@jrhender jrhender left a comment

Choose a reason for hiding this comment

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

We should specific that it is a Unix timestamp (which I think is the case), the start date from which the milliseconds are added is important

@jrhender
Copy link
Collaborator

Actually, I see that a Unix timestamp is in seconds not miliseconds, so we can't say that is directly a Unix timestamp. Perhaps we should say that it is a "Unix timestamp expressed in milliseconds"?

@jrhender
Copy link
Collaborator

@whitneypurdum In this PR, @JGiter is using seconds as the unit for RegisterOnchainOptions which I think might be correct https://github.com/energywebfoundation/iam-client-lib/pull/636/files
What are your thoughts?

@whitneypurdum
Copy link
Member Author

@jrhender so just in registerOnChainClaim
Options is it seconds?

@whitneypurdum
Copy link
Member Author

@whitneypurdum In this PR, @JGiter is using seconds as the unit for RegisterOnchainOptions which I think might be correct https://github.com/energywebfoundation/iam-client-lib/pull/636/files What are your thoughts?

updated so RegisterOnChainOptions is in seconds.

@jrhender
Copy link
Collaborator

@whitneypurdum I'm sorry, I'm looking at it again in and I think maybe it should be milliseconds in RegisterOnchainOptions and it is wrong in https://github.com/energywebfoundation/iam-client-lib/pull/636/files
Maybe we merge with milliseconds in this PR and then we can debate seconds vs milliseconds in the other PR 😄

@whitneypurdum
Copy link
Member Author

@whitneypurdum I'm sorry, I'm looking at it again in and I think maybe it should be milliseconds in RegisterOnchainOptions and it is wrong in https://github.com/energywebfoundation/iam-client-lib/pull/636/files Maybe we merge with milliseconds in this PR and then we can debate seconds vs milliseconds in the other PR 😄

OKay @jrhender , I changed it back to milliseconds. We can't merge anyways till we figure out why the tests are failing. This PR doesn't have the updated packages and it is now failing. Maybe something wrong with cache server?

@whitneypurdum whitneypurdum force-pushed the #ICL-347 branch 2 times, most recently from f67ea7c to c7c5b17 Compare August 10, 2022 15:47
@whitneypurdum whitneypurdum mentioned this pull request Aug 11, 2022
Merged
5 tasks
@whitneypurdum
Copy link
Member Author

@jrhender Please can you review this one more time?

jrhender
jrhender approved these changes Aug 15, 2022
View reviewed changes
Copy link
Collaborator

@jrhender jrhender 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. Could you fix the merge conflicts when you have a moment please @whitneypurdum ?

@whitneypurdum whitneypurdum merged commit 9595c34 into develop Aug 15, 2022
ewf-devops pushed a commit that referenced this pull request Aug 15, 2022
@semantic-release-bot
chore(release): 6.0.1-alpha.2 [skip ci]
51e0921 
### [6.0.1-alpha.2](v6.0.1-alpha.1...v6.0.1-alpha.2) (2022-08-15)

### Bug Fixes

* **exp:** document expirationTimestamp ([#631](#631)) ([9595c34](9595c34))
@ewf-devops
Copy link

🎉 This PR is included in version 6.0.1-alpha.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

ewf-devops pushed a commit that referenced this pull request Aug 31, 2022
@semantic-release-bot
chore(release): 6.0.1 [skip ci]
552eb05 
### [6.0.1](v6.0.0...v6.0.1) (2022-08-31)

### Bug Fixes

* **exp:** document expirationTimestamp ([#631](#631)) ([9595c34](9595c34))
* **issue claim:** allow boolean as valid field ([#639](#639)) ([77eb426](77eb426))
* use resolveCredentialAndVerify to verify enrolment prerequisites ([#630](#630)) ([9208ad3](9208ad3))
@ewf-devops
Copy link

🎉 This PR is included in version 6.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jrhender jrhender deleted the #ICL-347 branch December 6, 2022 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhender jrhender jrhender approved these changes

@nichonien nichonien nichonien approved these changes

Assignees
No one assigned
Labels
released on @canary released on @latest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@whitneypurdum @jrhender @ewf-devops @nichonien