Aggregation Logic #144
Aggregation Logic #144
Conversation
…ain with off-chain standalone tests
…ndalone off chain test suite
|
Shall we update the tests before review?
Also we should double check the |
jayantk
left a comment
jayantk
left a comment
There was a problem hiding this comment.
ok i read through this carefully and it largely makes sense. left a couple comments. would really prefer to address anything nontrivial in a separate PR though. will follow up on slack to figure out the best next steps.
| /* Brute force validate small sizes via the 0-1 principle (with | ||
| additional information in the keys to validate stability as well). */ | ||
|
|
||
| for( int n=0; n<=24; n++ ) { |
There was a problem hiding this comment.
this could really use a comment describing how the entries of x[i] are bitpacked.
|
|
||
| int * z = sort_stable( x,n, y ); | ||
|
|
||
| /* Make sure that z is a permutation of input data */ |
There was a problem hiding this comment.
this code block is duplicated. could extract this out to a function that is called here and above.
| } | ||
|
|
||
| // too few valid quotes | ||
| ptr->num_qt_ = numv; // FIXME: TEMPORARY GRAFT (ALL RETURN PATHS SET NUM_QT TO SOMETHING SENSIBLE) |
There was a problem hiding this comment.
why is this temporary? seems like if we delete the old aggregation logic, this should still be set here?
| // CONDENSED AND/OR MADE MORE EFFICIENT. AND IT PROBABLY WILL | ||
| // REPLACED SOON BY THE VWAP MODEL UNDER DEVELOPMENT. IT IS KEPT HERE | ||
| // TO KEEP THE CURRENT VWAP CALCULATION AS CLOSE AS POSSIBLE TO WHAT | ||
| // HOW IT CURRENTLY WORKS. |
There was a problem hiding this comment.
let's actually do this comment and delete the old price model from below. we can keep the old twap computation and feed the new aggregate price / confidence into it. I believe most of the code below is dead.
jayantk
left a comment
jayantk
left a comment
There was a problem hiding this comment.
LGTM. We decided to put this in a separate branch (so we can hotfix main if necessary), so I think you can push this to a branch called v2 and we can start sending PRs against that.
ali-behjati
left a comment
ali-behjati
left a comment
There was a problem hiding this comment.
I read through the unused previous logic, I think it is safe to remove them. Let's discuss it.
| // too few valid quotes | ||
| ptr->num_qt_ = numv; // FIXME: TEMPORARY GRAFT (ALL RETURN PATHS SET NUM_QT TO SOMETHING SENSIBLE) | ||
| if ( numv == 0 || numv < ptr->min_pub_ ) { | ||
| ptr->agg_.status_ = PC_STATUS_UNKNOWN; |
There was a problem hiding this comment.
Do we want this behavior to happen here?
My expectation was that even if number of publishers is less min_pub we still process the price but set the status to unknown. Here it only changes the status and does not calculate the price.
| for( ; j > 0 && ptr->comp_[ aidx[ j - 1 ] ].agg_.price_ > prc; --j ) { // FIXME: O(N^2) | ||
| aidx[ j ] = aidx[ j - 1 ]; | ||
| } | ||
| aidx[ j ] = idx; |
There was a problem hiding this comment.
Few lines below (line below // zero quoters) It sets number of publishers to something other than what we like: publishers with price between [agg_price/5, agg_price*5]
|
Pushed as branch |
This PR includes only commits required to merge aggregation logic into the Pyth contract, these have been spliced out from #139 with only the required utilities for aggregation. This doesn't include utilities that aren't used as they can be included in future PRs.