ExocoreNetwork / exocore

5 stars 9 forks source link

[R4R] feat(oracle) #30

Closed leonz789 closed 2 months ago

leonz789 commented 2 months ago

Description

Oracle module is used to collect external prices from sources(offchain/onchain) outside of exocorechain. And the prices access served by oracle module will be used by other modules like operator for voting power calculation. Supersedes #6 , reconstruct the commits for easy to review.

commits:

  1. proto file definition and some ignite scalffold dependencies
  2. mainly generated files from proto definition
  3. implement keeper for state
  4. add tests for keeper
  5. add cache serves as memory storage for price collection across blocks
  6. add aggregator, this implements the main logic for price aggregation Screenshot 2024-04-03 at 03 48 43
leonz789 commented 2 months ago

ref: #24

leonz789 commented 2 months ago

Thanks @bwhour and @MaxMustermann2 for the pr review(#24 ), here's former comments i just copied here and corresponding replies:

  1. workflows fail: A: working

  2. currency concern for maps: A: currently there's no concurrent access, will update when we do the concurrency related performance update.

  3. 'If there exists a pSource with no prices, accessing pSource.Prices[0] will trigger panic' A: before the message be processed, it has been checked to make sure the pSource at least has one price

  4. 'Use string concatenation (validator + strconv. Itoa(int(pSource.SourceId))) to generate the mapped key, whether this key generation rule may cause key conflicts' A: won't be, it's like a uri each level will not be conflict under parent scope

  5. 'list4Calculator, list4Aggregator := w.f.filtrate(msg): if the filtrate method encounters any exceptions while processing a message (e.g., the message is formatted incorrectly), the current implementation will not catch or handle those exceptions' A: The format check is and should be done in the first place when the message start processing(have been done in the checkmsg)

  6. 'In the updatePriceAndPower method, when new prices and weights are added to roundPrices, if any exceptions are encountered (e.g., if pw.price or pw.power is nil), the code doesn't perform error handling' A: same as above

  7. len(r.prices) < cap(r.prices) Its a good practice taking takes into account not exceeding the preset capacity. Is it necessary to take into account other boundary conditions, such as validation of the input parameters ? A: this length check is meant to make sure we don't accept values more than the limitation we set, just use the capicity as a boundary limitation

  8. 'list4Calculator, list4Aggregator := w.f.filtrate(msg): if the filtrate method encounters any exceptions while processing a message (e.g., the message is formatted incorrectly), the current implementation will not catch or handle those exceptions' A: Format check is/should be done in the checkmsg and antehandler

  9. 'im.PSources = append(im.PSources, item.PSources...) This case is If there are new PSources to be added, it will add the entire PSources array to the existing entry, not just the new SourceId. If if the new PSources array contains some new and some old (already existing in the current entry) SourceIds, then the price sources associated with the old SourceId will be added repeatedly; over time, if new PSources are added frequently, the PSources array in each entry may become very large, affecting data processing and query efficiency' A: the cache is just used as temporary storage, we wanna it be fast, and for the case you mentioned here, the duplicated srouceIds has been checked and ignored before put into cache.

  10. 'For collection operations (Add operations), assuming that they always succeed without checking the return value can hide problems.' A: not sure what this meant for.

Others:

  1. error handling and logs A: has added some and updated with new commits, will check further

  2. Update status to enum instead of integer A: will update

leonz789 commented 2 months ago

For the workflow: Tests / test-unit-cover (pull_request) it seems that the workflow can't handle the 'monkey patch' well, the test itself is actually passed.