feat: improve queryClient #52

gyuguen · 2022-12-20T05:43:37Z

close #47

I improve like below:

Improved queryClient

The height used by queryClient is fixed.
- When oracle receives an event as a subscription, it gets the height from the event.
- When oracle receives a request from client, it gets the last block height from lightClient.
  - However, the height decreases by one.
  - The reason is for quick response.

gyuguen · 2022-12-20T05:45:24Z

event/event.go

 	ctypes "github.com/tendermint/tendermint/rpc/core/types"
 )

 type Event interface {
 	Name() string
 	GetEventQuery() string
-	EventHandler(event ctypes.ResultEvent) error
+	EventHandler(context.Context, ctypes.ResultEvent) error


All event handlers added context. Because it uses shared query height.

gyuguen · 2022-12-20T05:47:03Z

event/subscriber.go

+
+			height, err := strconv.ParseInt(tx.Events["tx.height"][0], 10, 64)
+			// If an error occurs, the context does not set the height.
+			if err == nil {


Event if an error occurs, the following logic is performed.

gyuguen · 2022-12-20T05:48:30Z

panacea/query_client.go

@@ -38,14 +38,15 @@ import (

 type QueryClient interface {


Context is added to every call query in the blockchain.

gyuguen · 2022-12-20T05:50:23Z

panacea/query_client.go

@@ -306,6 +296,13 @@ func (q verifiedQueryClient) GetStoreData(ctx context.Context, storeKey string,
 	return result.Response.Value, nil
 }

+func (q verifiedQueryClient) getQueryBlockHeight(ctx context.Context) (int64, error) {


Gets the height from the context, if the height exists in the context.
If notm get last block height from lightClient.

It would be greater if we can do this without using context, since many practices are saying it's better to minimize the use of context (for clarity of source code).
If we can pass a targetHeight parameter to the GetStoreData() function, it would be enough.

OK. That would be good too.
If so, I will remove all contexts and modify it to specify the height only where it is needed.

I simply developed the function to be commonly applicable.

I would like to be able to set the height selectively for each logic.

However, I have a part that worries me.

The height to be used in the event and the height to be used in the client are currently implemented differently.
To speed up the client's response, the previous height of the last block was set.
The event uses the height passed from the event.

To support both of these, the following changes must be made.

type QueryClient interface { Close() error GetAccount(queryHeight int64, address string) (authtypes.AccountI, error) GetOracleRegistration(queryHeight int64, uniqueID string, address string) (*oracletypes.OracleRegistration, error) GetLightBlock(height int64) (*tmtypes.LightBlock, error) GetCdc() *codec.ProtoCodec GetChainID() string GetOracleParamsPublicKey(queryHeight int64) (*btcec.PublicKey, error) GetDeal(queryHeight int64, dealID uint64) (*datadealtypes.Deal, error) GetCertificate(queryHeight int64, dealID uint64, dataHash string) (*datadealtypes.Certificate, error) GetLastBlockHeight() (int64, error) }

Hmm. If you think we need to change many source codes for using queryHeight, you can just use context as you did. I read this PR again, and it looks reasonable. Sorry for bothering you. So, the decision is up to you.

I have been thinking about this part, and the conclusion will be modified so that the height is input as @youngjoon-lee initially suggested.
Using Middleware is convenient, but because it is hidden code, someone else brings confusion.

Ah. I also forgot that jwtAuthMiddleware also uses verifiedQueryClient.
https://github.com/medibloc/panacea-oracle/pull/52/files#diff-6f125263042ee6bd63bdbbe9ea792e6d409132c4e91fc0e4553c668e9b9d99edR74

Then, it's more reasonable to use context to pass the query height to the jwtAuthMiddleware. Anyway, I'd like to say that the decision is up to you :)

Ok, I'll decide to leave the code as is.

If I think this code is wrong, I'll fix it later. :)

gyuguen · 2022-12-20T05:52:27Z

panacea/tx.go

@@ -56,7 +58,7 @@ func (tb TxBuilder) GenerateSignedTxBytes(
 		return nil, err
 	}

-	signerAccount, err := tb.client.GetAccount(signerAddress)
+	signerAccount, err := tb.client.GetAccount(context.Background(), signerAddress)


Maybe it will be change to grpcClient.

gyuguen · 2022-12-20T05:53:47Z

server/middleware/query.go

+		if err == nil {
+			log.Debugf("Set the previous value of the last block heignt. LastHeight: %v, SetHeight: %v", height, height-1)
+			r = r.WithContext(
+				panacea.SetQueryBlockHeightToContext(r.Context(), height-1),


I used the previous height of the last block

Could you explain why do you use the previous height of the latest block here?
Is it for (almost) immediate response?

Yes that's the biggest reason.
And it doesn't seem to be a query that is sensitive to block height.

I think it is very rare case, but what if there is no data in the previous block? since the data is set in the latest block.
In other words, it's the case that a transaction (to store data) is executed and somebody query the data at the same time. We don't have to care this edge case?

Yes, that case can happen enough.
But I think we should put more importance on quick response.
I think a late response will cause more problems.

Ah. I haven't thought about using the previous height of the latest block. Umm... it's a trade-off as you and Hansol said..... Accuracy vs Latency..
Let me think about it..

gyuguen · 2022-12-20T05:56:13Z

server/server.go


 	dealRouter := router.PathPrefix("/v0/data-deal").Subrouter()
-	dealRouter.Use(jwtAuthMiddleware.Middleware)
+	dealRouter.Use(
+		queryMiddleware.Middleware,


This order is important.
You must first set the height before using the query.

0xHansLee

I tested and confirmed that it is improved very well. Good work!

audtlr24

Thanks, lgtm!

server/middleware/query.go

youngjoon-lee

lgtm

gyuguen added 2 commits December 20, 2022 14:22

feat: improve queryClient

1c66d95

fix

6c8580e

gyuguen added this to the v2.1.0-beta milestone Dec 20, 2022

gyuguen requested review from youngjoon-lee, audtlr24, 0xHansLee and inchori December 20, 2022 05:43

gyuguen self-assigned this Dec 20, 2022

gyuguen commented Dec 20, 2022

View reviewed changes

0xHansLee approved these changes Dec 20, 2022

View reviewed changes

audtlr24 approved these changes Dec 20, 2022

View reviewed changes

youngjoon-lee reviewed Dec 21, 2022

View reviewed changes

server/middleware/query.go Outdated Show resolved Hide resolved

youngjoon-lee reviewed Dec 21, 2022

View reviewed changes

server/middleware/query.go Outdated Show resolved Hide resolved

gyuguen added 3 commits December 21, 2022 12:30

fix

6fff097

Merge branch 'main' into ft/47/improve_query_client

0c841b6

Merge branch 'main' into ft/47/improve_query_client

ce66f54

youngjoon-lee approved these changes Dec 21, 2022

View reviewed changes

gyuguen merged commit f4e07b3 into main Dec 21, 2022

gyuguen deleted the ft/47/improve_query_client branch December 21, 2022 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve queryClient #52

feat: improve queryClient #52

gyuguen commented Dec 20, 2022

gyuguen Dec 20, 2022

gyuguen Dec 20, 2022

gyuguen Dec 20, 2022

gyuguen Dec 20, 2022

youngjoon-lee Dec 20, 2022 •

edited

Loading

gyuguen Dec 20, 2022

gyuguen Dec 20, 2022

gyuguen Dec 20, 2022 •

edited

Loading

youngjoon-lee Dec 21, 2022

gyuguen Dec 21, 2022 •

edited

Loading

youngjoon-lee Dec 21, 2022

gyuguen Dec 21, 2022

gyuguen Dec 20, 2022

gyuguen Dec 20, 2022

0xHansLee Dec 20, 2022

gyuguen Dec 20, 2022

0xHansLee Dec 20, 2022

gyuguen Dec 20, 2022

youngjoon-lee Dec 21, 2022 •

edited

Loading

gyuguen Dec 20, 2022

0xHansLee left a comment

audtlr24 left a comment

youngjoon-lee left a comment

feat: improve queryClient #52

feat: improve queryClient #52

Conversation

gyuguen commented Dec 20, 2022

Improved queryClient

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

youngjoon-lee Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuguen Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuguen Dec 21, 2022 • edited Loading

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

youngjoon-lee Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xHansLee left a comment

Choose a reason for hiding this comment

audtlr24 left a comment

Choose a reason for hiding this comment

youngjoon-lee left a comment

Choose a reason for hiding this comment

youngjoon-lee Dec 20, 2022 •

edited

Loading

gyuguen Dec 20, 2022 •

edited

Loading

gyuguen Dec 21, 2022 •

edited

Loading

youngjoon-lee Dec 21, 2022 •

edited

Loading