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

Added of date for data commit entry to endpoint "ack" #815

Conversation

ilzheev
Copy link
Contributor

@ilzheev ilzheev commented Jul 25, 2019

Our core developer added transactiondate & transactiondatestring into factomd API ack response.

So, ack of entry now contains the timestamp of entry creation (transaction).
Looks like this timestamp existed in entry-ack, but was lost during refactoring from entry-ackack, now we return it back.

I have tested it and it works correctly.

Example of improved API response:

{"jsonrpc":"2.0","id":0,"result":{"committxid":"7a86efb415f4eecb3eb2c7733e3fa7edc01541b8dcbc82700e06d19d283fed39","entryhash":"0d835d5f3a9ef33b1955506d6c452430f4aa5f839d98e0a75a58249829260987","commitdata":{"transactiondate":1564044424749,"transactiondatestring":"2019-07-25 12:47:04","status":"DBlockConfirmed"},"entrydata":{"blockdate":1564044300,"blockdatestring":"2019-07-25 12:45:00","status":"DBlockConfirmed"}}}

@PaulBernier
Copy link
Contributor

the datestring is redundant information with the timestamp and less flexible. It should be the responsibility of the client to format the time to a string as it pleases him/her. Basically I think the datestring should be simply dropped.

@ilzheev
Copy link
Contributor Author

ilzheev commented Jul 25, 2019

the datestring is redundant information with the timestamp and less flexible. It should be the responsibility of the client to format the time to a string as it pleases him/her. Basically I think the datestring should be simply dropped.

I agree with that, but from the consistency side it should be there.
There is datestring for entrydata:


"entrydata": {
"blockdate":1564044300,
"blockdatestring":"2019-07-25 12:45:00",

So, Michael added transactiondate & transactiondatestring, so both commitdata & entrydata will be consistent.

@ilzheev
Copy link
Contributor Author

ilzheev commented Jul 25, 2019

We are preparing a proposal for refactoring of ack call, the drop of datestring's should be discussed there IMO.

@PaulBernier
Copy link
Contributor

PaulBernier commented Aug 12, 2019

Oh, ok, I didn't realize that there was originally already a datestring field.

@stackdump stackdump changed the base branch from FD-851_release_candidate_xuan to FD-1120_xuan_rollup_2 August 26, 2019 17:16
@stackdump stackdump merged commit 38268e1 into FactomProject:FD-1120_xuan_rollup_2 Aug 26, 2019
@Emyrk
Copy link
Contributor

Emyrk commented Aug 26, 2019

LG

@ilzheev
Copy link
Contributor Author

ilzheev commented Sep 29, 2019

@stackdump @Emyrk Hey, I'm testing v6.4.2-rc2-alpine and looks like this PR is not included there. Can we include it into final Xuan release?

@stackdump
Copy link
Contributor

added another pr: #885 to pull this commit into rollup 3

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

5 participants