[R4R] feat(oracle) #24
Conversation
…lated data in keeper
… scalffold dependencies
|
Can you please fix the failing workflows? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The status field uses an integer type to indicate status (1 for open, 2 for closed). Using enumerated types or constant definitions instead of magic numbers makes the code easier to understand and maintain.
There was a problem hiding this comment.
The AggregatorContext contains multiple mappings (e.g. validatorsPower, rounds, aggregators) that may be accessed and modified concurrently in a multi-threaded environment. If this is the case, you need to ensure that access to these mappings is thread-safe and may need to use mutex locking or other concurrency control mechanisms
There was a problem hiding this comment.
Concurrency control needs to be considered when multiple goroutines may access or modify the deterministicSource mapping in the calculator. it is a common requirement to handle concurrency in applications like blockchain and price aggregation.
There was a problem hiding this comment.
If there exists a pSource with no prices, accessing pSource.Prices[0] will trigger panic
There was a problem hiding this comment.
Use string concatenation (validator + strconv. Itoa(int(pSource.SourceId))) to generate the mapped key, whether this key generation rule may cause key conflicts
There was a problem hiding this comment.
For collection operations (Add operations), assuming that they always succeed without checking the return value can hide problems.
x/oracle/keeper/aggregator/worker.go
Outdated
There was a problem hiding this comment.
Setting f, c, and a to nil in the seal method may help with garbage collection, but if there are operations in these objects that require explicit shutdown or cleanup of resources, extra care is needed to ensure that the resources are freed properly
x/oracle/keeper/aggregator/worker.go
Outdated
There was a problem hiding this comment.
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
| storeKey storetypes.StoreKey | ||
| memKey storetypes.StoreKey | ||
| paramstore paramtypes.Subspace | ||
| stakingKeeper stakingkeeper.Keeper |
There was a problem hiding this comment.
Please convert this to the minimum required interface in expected_keepers.go.
|
|
||
| keeper keeper.Keeper | ||
| accountKeeper types.AccountKeeper | ||
| bankKeeper types.BankKeeper |
There was a problem hiding this comment.
Can you add a comment here to say that they are only used in the simulation?
There was a problem hiding this comment.
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。
There was a problem hiding this comment.
functions in this cache file seems no error handling and logging logic.
| price.NextRountId = binary.BigEndian.Uint64(iterator.Value()) | ||
| } else { | ||
| var val types.PriceWithTimeAndRound | ||
| k.cdc.Unmarshal(iterator.Value(), &val) |
There was a problem hiding this comment.
k.cdc.Unmarshal has error return which need to be handle. can k.cdc.MustMarshal ?
| return | ||
| } | ||
|
|
||
| k.cdc.Unmarshal(b, &price) |
There was a problem hiding this comment.
k.cdc.Unmarshal has error return which need to be handle. can k.cdc.MustMarshal ?
| b := store.Get(types.PricesRoundKey(nextRoundId - 1)) | ||
| if b != nil { | ||
| //should always be true | ||
| k.cdc.Unmarshal(b, &price) |
There was a problem hiding this comment.
k.cdc.Unmarshal has error return which need to be handle. can k.cdc.MustMarshal ?
| return | ||
| } | ||
| } | ||
| if len(r.prices) < cap(r.prices) { |
There was a problem hiding this comment.
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 ?
|
|
||
| // udpate priceAndPower for a specific DSRoundID, if the price exists, increase its power with provided data | ||
| // return confirmed=true, when detect power exceeds the threshold | ||
| func (r *roundPrices) updatePriceAndPower(pw *priceAndPower, totalPower *big.Int) (updated bool, confirmed bool) { |
There was a problem hiding this comment.
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
|
Merging with the original branch introduced confusion due to too many commits. I will reconstruct it to create a cleaner version. Apologies for the mess. (will follow this 6-commit structure for easy review) |
|
I will close this pr, and #30 will take place for the oracle module feature pr. |
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: