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
feat: w3 aggregation #51
feat: w3 aggregation #51
Changes from 20 commits
60b1a2b
898f2eb
1c76c38
3e624b7
7248631
8286f15
d5cb712
1de9b86
58026c5
dae9ff4
91e9280
7695dc0
965ceed
49daef5
208e831
0213450
c7016e0
d4e8912
904e2d3
7dee532
5a50385
49f5704
ec12c09
4adee82
9b80833
0d2f464
babefc0
555d647
c2169a4
055603e
8c561b0
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 was under impression that in the last call we decided we don't want to link to the CAR containing these entries but rather have
offer
field contain these entries inline insteadThere 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, text above is changed to be a single block inline, but I forgot to change the JSON example here
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 consider two things here:
How does cashing affects this ? Ideally invocations are idempotent meaning same instruction should get same result (even if state has changed in the meantime) and
nnc
supposed to be utilized to cache miss. If we do not want to deal with caching we should specify additional field like current time and request that it be set to e.g. latest drand or a wall clock, that way out dated responses are less surprising, if you request state for old state that has not been computed prior you get an error saying it can not be computed, if we have it cached than response is for that time frame.We should specify what happens if you request deal info for the aggregate that is in
queued
state.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 if aggregate is queued or done, we do not want to do a new one and it should just be ignored? Therefore, it would be idempotent
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.
That is reasonable, lets just add sentence to state that.