[R4R] feat(oracle) #30

leonz789 · 2024-04-09T14:59:11Z

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:

proto file definition and some ignite scalffold dependencies
mainly generated files from proto definition
implement keeper for state
add tests for keeper
add cache serves as memory storage for price collection across blocks
add aggregator, this implements the main logic for price aggregation

github-advanced-security

gosec found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

x/oracle/types/info.pb.go

testutil/nullify/nullify.go

x/oracle/keeper/single.go

x/oracle/keeper/aggregator/aggregator.go

x/oracle/module.go

x/oracle/keeper/cache/caches.go

x/oracle/keeper/single.go

+		for b, recentParams := range recentParamsMap {
+			if b <= from && b > prev {
+				pTmp = common.Params(*recentParams)
+				agc.SetParams(&pTmp)
+				prev = b
+			}
+		}


leonz789 · 2024-04-09T15:15:26Z

ref: #24

x/oracle/keeper/prices.go

bwhour · 2024-04-11T13:53:47Z

x/oracle/keeper/prices.go

+
+	b := store.Get(types.PricesRoundKey(roundId))
+	if b == nil {
+		return


return uninitialized variables

expected, found will be initialized to false by default, just return the false is expected.

x/oracle/keeper/prices.go

leonz789 · 2024-04-11T15:55:31Z

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

workflows fail:
A: working
currency concern for maps:
A: currently there's no concurrent access, will update when we do the concurrency related performance update.
'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
'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
'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)
'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
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
'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
'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.
'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:

error handling and logs
A: has added some and updated with new commits, will check further
Update status to enum instead of integer
A: will update

x/oracle/client/cli/tx_create_price.go

x/oracle/keeper/aggregator/aggregator.go

x/oracle/keeper/cache/caches.go

+		for addr, power := range c.validators.validators {
+			item[addr] = power
+		}


x/oracle/keeper/aggregator/aggregator.go

x/oracle/types/params.go

+
+// String implements the Stringer interface.
+func (p Params) String() string {
+	out, _ := yaml.Marshal(p)


x/oracle/module.go

+	if len(validatorUpdates) > 0 {
+		validatorList := make(map[string]*big.Int)
+		for _, vu := range validatorUpdates {
+			pubKey, _ := cryptocodec.FromTmProtoPublicKey(vu.PubKey)


x/oracle/types/message_update_params.go

+
+// GetSigners returns the expected signers for a MsgUpdateParams message
+func (msg *MsgUpdateParams) GetSigners() []sdk.AccAddress {
+	addr, _ := sdk.AccAddressFromBech32(msg.Authority)


x/oracle/keeper/common/types.go

x/oracle/keeper/aggregator/aggregator.go

x/oracle/client/cli/tx_create_price.go

x/oracle/keeper/single.go

+				agc.FillPrice(&types.MsgCreatePrice{
+					Creator:  msg.Validator,
+					FeederID: msg.FeederID,
+					Prices:   msg.PSources,
+				})


leonz789 · 2024-04-17T13:57:17Z

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.

… scalffold dependencies

…price update

… across blocks

…lag for monkey

TimmyExogenous

There is a lot of code in this PR, so didn't review it very deeply, just left some questions.

proto/exocore/oracle/info.proto

proto/exocore/oracle/query.proto

proto/exocore/oracle/recent_params.proto

proto/exocore/oracle/token_feeder.proto

x/oracle/keeper/aggregator/aggregator.go

x/oracle/keeper/common/types.go

x/oracle/keeper/single.go

x/oracle/keeper/aggregator/aggregator_aggregator.go

x/oracle/keeper/aggregator/aggregator.go

mikebraver · 2024-04-22T11:58:14Z

proto/exocore/oracle/genesis.proto

+
+  //TODO: userDefinedTokenFeeder
+  // latest block on which the validator set be updated
+  ValidatorUpdateBlock validator_update_block = 4;


Should this start from 3?

x/oracle/keeper/aggregator/context.go

x/oracle/keeper/aggregator/aggregator.go

x/oracle/keeper/aggregator/context.go

+	for feederID, round := range agc.rounds {
+		if round.status == 1 {
+			feeder := agc.params.GetTokenFeeder(feederID)
+			// TODO: for mode=1, we don't do aggregate() here, since if it donesn't success in the transaction execution stage, it won't success here
+			// but it's not always the same for other modes, switch modes
+			switch common.Mode {
+			case 1:
+				expired := feeder.EndBlock > 0 && uint64(ctx.BlockHeight()) >= feeder.EndBlock
+				outOfWindow := uint64(ctx.BlockHeight())-round.basedBlock >= uint64(common.MaxNonce)
+				if expired || outOfWindow || force {
+					failed = append(failed, feeder.TokenID)
+					if expired {
+						delete(agc.rounds, feederID)
+						delete(agc.aggregators, feederID)
+					} else {
+						round.status = 2
+						agc.aggregators[feederID] = nil
+					}
+				}
+			default:
+				ctx.Logger().Info("mode other than 1 is not support now")
+			}
+		}
+		// all status: 1->2, remove its aggregator
+		if agc.aggregators[feederID] != nil && agc.aggregators[feederID].sealed {
+			agc.aggregators[feederID] = nil
+		}
+	}


x/oracle/keeper/aggregator/context.go

MaxMustermann2 · 2024-04-23T19:13:14Z

app/app.go

@@ -448,15 +455,12 @@ func NewExocoreApp(
 		operatorTypes.StoreKey,
 		avsManagerTypes.StoreKey,
 		avsTaskTypes.StoreKey,
+		oracleTypes.StoreKey,
 	)

 	// Add the EVM transient store key


Update or remove this comment.

MaxMustermann2 · 2024-04-23T19:14:58Z

app/app.go

@@ -977,6 +985,7 @@ func NewExocoreApp(
 		withdrawTypes.ModuleName,
 		rewardTypes.ModuleName,
 		exoslashTypes.ModuleName,
+		oracleTypes.ModuleName,


Please explain the rationale for these orders, if there is any. Or add a comment anyway if there is no particular rationale.

MaxMustermann2 · 2024-04-23T19:16:19Z

app/app.go

-		// nolint: staticcheck
-		WithKeyTable(evmtypes.ParamKeyTable())
+	paramsKeeper.Subspace(evmtypes.ModuleName).WithKeyTable(evmtypes.ParamKeyTable()) //nolint:staticcheck
+	paramsKeeper.Subspace(oracleTypes.ModuleName).WithKeyTable(oracleTypes.ParamKeyTable())


Shouldn't this also have the same nolint line ?

MaxMustermann2 · 2024-04-23T19:20:34Z

proto/exocore/oracle/price.proto

+  int32 decimal = 2;
+  // timestamp when the price corresponding to
+  string timestamp = 3;
+  // det_id is used for deterministic source to tell of which round from this soure the price is corresponded


typo: soure

MaxMustermann2 · 2024-04-23T19:22:32Z

tools/tools.go

+	_ "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway"
+	_ "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger"
+	_ "github.com/grpc-ecosystem/grpc-gateway/v2/protoc-gen-openapiv2"
+)


MaxMustermann2 · 2024-04-23T21:06:01Z

x/oracle/keeper/common/types.go

+	return s.size
+}
+
+func NewSet[T comparable](length int) *Set[T] {


MaxMustermann2 · 2024-04-23T21:08:31Z

x/oracle/keeper/prices.go

+}
+
+// RemovePrices removes a prices from the store
+func (k Keeper) RemovePrices(


Can we do some pruning of this data if it gets old ?

Yes, we can add the pruning feature as config in further PR.

MaxMustermann2 · 2024-04-23T21:09:48Z

x/oracle/keeper/query_prices.go

+		return nil, status.Error(codes.NotFound, "not found")
+	}
+
+	return &types.QueryGetPricesResponse{Prices: val}, nil


Yes, i will update in a new PR with related test

MaxMustermann2 · 2024-04-23T21:13:46Z

x/oracle/keeper/recent_msg.go

@@ -0,0 +1,77 @@
+package keeper


When is the data used? Can you explain briefly?

This is used for refill the cache which stores price messages for aggregation:
since we have a window of more than one block to collect the prices from all validators, if this process is interrupted before the final aggregated price written to block, we would like to have some way to retrieve these messages. After we got the final price, the messages could be able to remove, so we just need the recent messages within the window.

MaxMustermann2 · 2024-04-23T21:13:51Z

x/oracle/keeper/recent_params.go

@@ -0,0 +1,76 @@
+package keeper


When is the data used? Can you explain briefly?

This is like the recent_msgs, which is used to refill the cache:
the aggregation is affected by 3 elements:

validatorset(: votingPower)

params(: tokenFeeder)

messages (: including price_with_votingPower for tokenFeeder)
So when we do the refill, we just use these 3 data to reproduce the aggregation process.
But i think the recent_params could be optimized with carefully check like validator_update_block so that we don't need to keep the history params, currently this is a straightforward way. I will update in a new PR

mikebraver · 2024-04-24T09:33:48Z

x/oracle/keeper/single.go

+	validatorPowers := make(map[string]*big.Int)
+	k.IterateBondedValidatorsByPower(ctx, func(_ int64, validator stakingtypes.ValidatorI) bool {
+		power := big.NewInt(validator.GetConsensusPower(validator.GetBondedTokens()))
+		// addr := string(validator.GetOperator())


Delete unnecessary comment

mikebraver · 2024-04-24T10:33:20Z

x/oracle/keeper/recent_params.go

+
+// SetRecentParams set a specific recentParams in the store from its index
+func (k Keeper) SetRecentParams(ctx sdk.Context, recentParams types.RecentParams) {
+	store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefix(types.RecentParamsKeyPrefix))


Is this only used in genesis initialize?

No, this is used to refill the cache

mikebraver · 2024-04-24T13:52:49Z

x/oracle/keeper/msg_server_create_price.go

+)
+
+// CreatePrice proposes price for new round of specific tokenFeeder
+func (ms msgServer) CreatePrice(goCtx context.Context, msg *types.MsgCreatePrice) (*types.MsgCreatePriceResponse, error) {


Wouldn't this be called by validator?

This serves a a message service, and the message should be signed by validator

mikebraver

fix the action error then

github-actions bot added C:Proto C:CLI Type: Build Type: Docs labels Apr 9, 2024

github-advanced-security bot found potential problems Apr 9, 2024

View reviewed changes

leonz789 requested review from rlan35, bwhour, MaxMustermann2, mikebraver and TimmyExogenous April 9, 2024 15:10

leonz789 mentioned this pull request Apr 9, 2024

[R4R] feat(oracle) #24

Closed

bwhour reviewed Apr 11, 2024

View reviewed changes

x/oracle/keeper/prices.go Outdated Show resolved Hide resolved

bwhour reviewed Apr 11, 2024

View reviewed changes

x/oracle/keeper/prices.go Show resolved Hide resolved

bwhour reviewed Apr 11, 2024

View reviewed changes

x/oracle/keeper/prices.go Outdated Show resolved Hide resolved

leonz789 force-pushed the develop-oracle-review branch from 6ebdc14 to ede2652 Compare April 13, 2024 14:42

github-advanced-security bot found potential problems Apr 14, 2024

View reviewed changes

x/oracle/client/cli/tx_create_price.go Fixed Show fixed Hide fixed

github-actions bot removed Type: Build Type: Docs labels Apr 16, 2024

github-advanced-security bot found potential problems Apr 17, 2024

View reviewed changes

leonz789 added 5 commits April 18, 2024 10:46

feat(oracle-proto): add proto files for oracle module and some ignite…

24fefab

… scalffold dependencies

feat(oracle-proto): add oracle types, mainly proto generated files

d78044c

feat(oracle-keeper):implement keeper for state access, msgserver for …

afb4965

…price update

test(oracle-keeper):add test

d6d244f

feat(oracle-keeper):add cache as memory storage for prices collection…

9809ec5

… across blocks

leonz789 added 6 commits April 18, 2024 10:46

text(oracle-proto): proto lint, PriceWithSource->PriceSource

cb6e88c

text(oracle-proto): proto lint, PriceWithTimeAndRound->PriceTimeRound

ecfda24

text(oracle-proto): proto lint, RuleWithSource->RuleSource

dfb42a4

get rid of local test replace

2ef42cd

get rid of integer conversion

a723d1a

add some TODO comments for test

dc84085

leonz789 force-pushed the develop-oracle-review branch from ed1e350 to dc84085 Compare April 18, 2024 02:48

leonz789 added 2 commits April 18, 2024 18:22

test(oracle-keeper):reset caches betweent testsuite

5567367

test(oracle)change monkey to gomonkey for arch compatible, add test f…

e6c3c85

…lag for monkey

github-actions bot added the Type: Build label Apr 18, 2024

ExocoreNetwork deleted a comment from qinglin89 Apr 18, 2024

bwhour approved these changes Apr 22, 2024

View reviewed changes

TimmyExogenous reviewed Apr 22, 2024

View reviewed changes

mikebraver reviewed Apr 22, 2024

View reviewed changes

refactor: udpate the file name of aggregator, and fix typo

06f5fd4

github-advanced-security bot found potential problems Apr 22, 2024

View reviewed changes

TimmyExogenous approved these changes Apr 23, 2024

View reviewed changes

fix proto index

7c3dff1

MaxMustermann2 reviewed Apr 23, 2024

View reviewed changes

mikebraver reviewed Apr 24, 2024

View reviewed changes

fix typo and comments

7588a48

mikebraver approved these changes Apr 25, 2024

View reviewed changes

disable oracle module temporary

8cc8f75

MaxMustermann2 approved these changes Apr 25, 2024

View reviewed changes

leonz789 added 2 commits April 25, 2024 18:30

enable oracle module in app, and set default params to a future block

ecf9145

fix lint

d892e60

leonz789 merged commit 9f1b6a8 into ExocoreNetwork:develop Apr 25, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R4R] feat(oracle) #30

[R4R] feat(oracle) #30

leonz789 commented Apr 9, 2024

github-advanced-security bot left a comment

leonz789 commented Apr 9, 2024

bwhour Apr 11, 2024

leonz789 Apr 18, 2024

leonz789 commented Apr 11, 2024 •

edited

Loading

leonz789 commented Apr 17, 2024

TimmyExogenous left a comment

mikebraver Apr 22, 2024

leonz789 Apr 23, 2024

MaxMustermann2 Apr 23, 2024

leonz789 Apr 25, 2024

MaxMustermann2 Apr 23, 2024

leonz789 Apr 25, 2024

MaxMustermann2 Apr 23, 2024

leonz789 Apr 25, 2024

MaxMustermann2 Apr 23, 2024

leonz789 Apr 25, 2024

MaxMustermann2 Apr 23, 2024

MaxMustermann2 Apr 23, 2024

MaxMustermann2 Apr 23, 2024

leonz789 Apr 25, 2024

MaxMustermann2 Apr 23, 2024

leonz789 Apr 25, 2024

MaxMustermann2 Apr 23, 2024

leonz789 Apr 25, 2024

MaxMustermann2 Apr 23, 2024

leonz789 Apr 25, 2024

mikebraver Apr 24, 2024

leonz789 Apr 25, 2024

mikebraver Apr 24, 2024

leonz789 Apr 25, 2024

mikebraver Apr 24, 2024

leonz789 Apr 25, 2024

mikebraver left a comment

[R4R] feat(oracle) #30

[R4R] feat(oracle) #30

Conversation

leonz789 commented Apr 9, 2024

Description

github-advanced-security bot left a comment

Choose a reason for hiding this comment

leonz789 commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonz789 commented Apr 11, 2024 • edited Loading

leonz789 commented Apr 17, 2024

TimmyExogenous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebraver left a comment

Choose a reason for hiding this comment

leonz789 commented Apr 11, 2024 •

edited

Loading