Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
API v2 specification along with the new requirements #300
API v2 specification along with the new requirements #300
Changes from 3 commits
3b736eb
063c964
090594f
2485b17
f4b4a86
7145b78
2dd0cd8
c3aca8c
cc61f8e
4217e64
bcf10c5
0ea7633
babbcee
39e5d8f
8bf4fdb
56cef0b
44131e9
356967c
ec026ea
24b2b30
3fef9a1
1a4b69a
f42ab30
47d1c76
7698cda
3a80bb0
2d8ac12
4b804d4
a316deb
28f69e2
1a80a40
d2c765a
d5f6f85
88b67dc
5df3172
194553a
c2aee87
5c8fe0f
0d45302
52e1aa5
47f0e8a
71210bf
ef8ca21
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to split block into block header and full block. Header includes a list of TXIDs, but not full transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In v1, full transactions are sent within a block. I'm not sure if we want to change that. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like this data structure is actually unused in this design, since
Layer
contains only a block. this should be fixed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
hash
androot_state_hash
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add something like
current_layer
which is the current layer number based strictly on timeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in v1 we have
synced_layer
- latest layer we saw from the networktop_layer
- current layer, based on timeverified_layer
- latest verified layerwhat is your proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're missing one. Here's my proposal:
synced_layer
is only useful while syncing. Blocks are also received via gossip. The relationship should be:synced_layer <= verified_layer <= head_layer <= current_layer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to also add recipient and send amount (for Send txs). I'm not 100% sure how to structure this. we should probably add a subfield, something liked ParsedContents, that's
oneof
depending on the type of tx (spawn, self spawn, send, etc.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can also left that to user. In explorer I'm parsing every tx from raw format, but if you want to put everything as fields we can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is an important requirement for consumers. We shouldn't require them to parse a raw TX, the API should provide the parsed TX. If for some reason we think this will be expensive, we can add a flag to the Request object to enable this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to double check but pretty sure this is unused, not sure we want to include this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmed, layer limits for tx are not currently enabled. let's remove this for now for simplicity and to avoid confusion.