zoneconcierge: proper initialisation for chain info #285

SebastianElvis · 2023-01-17T22:29:20Z

Fixes an improvement found by @gusin13

This PR introduces the proper initialisation for chain info.

Previously, upon querying GetChainInfo, if a chain info does not exist, then ZoneConcierge returns an empty chain info rather than an error. This is not proper since the function has to give a clear message on whether a chain info exists or not. For example, one side effect of this is that the chain info API returns 200 even if the chain info does not exist.

This PR introduces the initialisation that works as follows. Upon a header via IBC

if chain info does not exist, initialise an empty one
if canonical header, then update the latest header to this one and increment counter
if fork header, then update the fork header as before

vitsalis

Good stuff. I'm not entirely onboard with the new errors introduced here and the way they are handled, added corresponding comments

vitsalis · 2023-01-18T07:13:34Z

x/zoneconcierge/keeper/chain_info_indexer.go

+	if err != nil {
+		// chain info has not been initialised yet
+		// this can only be a programming error
+		panic(err)


Here we panic since not having a ChainInfo can only be a programming error. However, on the previous if condition, we return an error. Why don't we panic there as well or return an error?

Given that this is a helper method and it doesn't have a complete view of the entire logic, I would avoid panicking in this function as the reader does not have all the context.

Good catch! After a second thought, maybe let's return an error here. It will be the AfterHeaderWithValidCommit hook's responsibility to panic.

vitsalis · 2023-01-18T07:13:58Z

x/zoneconcierge/keeper/chain_info_indexer.go

-	chainInfo := k.GetChainInfo(ctx, chainID)
+	chainInfo, err := k.GetChainInfo(ctx, chainID)
+	if err != nil {
+		return sdkerrors.Wrapf(types.ErrChainInfoNotFound, "cannot insert fork header when chain info is not initialized")


In updateLatestHeader we panic. What's the difference here?

Have updated the PR to return errors in tryToUpdateLatestForkHeader and updateLatestHeader consistently 👍

vitsalis · 2023-01-18T07:16:05Z

x/zoneconcierge/keeper/hooks.go

+
+	// initialise chain info if not exist
+	chainInfo, err := h.k.GetChainInfo(ctx, indexedHeader.ChainId)
+	if err != nil {


What if there's another kind of error related to GetChainInfo? I suggest that GetChainInfo returns nil if no chain info exists (or we implement a method ChainInfoExists) and we create that in this case. In the future, if an extra error is created this might spill over here and we panic for no reason.

Good point. I have changed the PR so that we initialise chain info only when the error is of type ErrEpochChainInfoNotFound, and panic directly if the error is other types.

I suggest that GetChainInfo returns nil if no chain info exists (or we implement a method ChainInfoExists) and we create that in this case.

I remember we had a discussion on this and the conclusion was that we need to have proper initialisation of chain info and give the invoker a clear message if the chain info does not exist. This was part of the motivation of this PR actually.

vitsalis · 2023-01-18T07:17:20Z

x/zoneconcierge/keeper/chain_info_indexer.go

+		return nil, fmt.Errorf("chainID is empty")
+	}
+	// ensure chain info has not been initialised yet
+	if _, err := k.GetChainInfo(ctx, chainID); err == nil {


The GetChainInfo returning an error is not a good condition for identifying whether a Chain Info exists imo. I would have it return nil if no such info exists.

Like above, I updated the PR to distinguish errors by using sdkerrors.IsOf.

If other errors occur maybe AfterHeaderWithValidCommit will need to do other corresponding things, e.g., panic, rather than just returning nil. Wdyt?

It took me a while to get around why we return an error under err==nil, so, I would suggest we don't use k.GetChainInfo() here. Instead, we could have a k.HasChainInfo() method that wraps store.Has(). k.GetChainInfo() can reuse k.HashChainInfo() and calling k.HasChainInfo() would be more efficient as it does not need to read the database and unmarshal the object.

Yep that's a good idea. Fixed 👍

vitsalis · 2023-01-18T07:17:50Z

x/zoneconcierge/keeper/chain_info_indexer.go

-				Headers: []*types.IndexedHeader{},
-			},
-		}
+		return nil, types.ErrEpochChainInfoNotFound


Why return an error here? I don't think it's an error if no chain info exists.

It was not an error before this PR. This PR aims at introducing initialisation of chain info, such that the invokers of GetChainInfo gets a clear message whether the chain checkpoints to Babylon or not. For example, without this PR, the chain_info API always returns status code 200 even if the chain does not checkpoint to Babylon. Instead in this case, the status code should be 500.

SebastianElvis · 2023-01-23T05:47:56Z

Thanks Vitalis for the great comments! I have addressed all of them. Since Vitalis is offline this week, @gitferry could you please have a look at this PR? Thanks!

gitferry

Great work! Left a few suggestions:

gitferry · 2023-01-23T14:10:07Z

x/zoneconcierge/keeper/chain_info_indexer.go

+		return nil, fmt.Errorf("chainID is empty")
+	}
+	// ensure chain info has not been initialised yet
+	if _, err := k.GetChainInfo(ctx, chainID); err == nil {


It took me a while to get around why we return an error under err==nil, so, I would suggest we don't use k.GetChainInfo() here. Instead, we could have a k.HasChainInfo() method that wraps store.Has(). k.GetChainInfo() can reuse k.HashChainInfo() and calling k.HasChainInfo() would be more efficient as it does not need to read the database and unmarshal the object.

gitferry · 2023-01-23T14:20:19Z

x/zoneconcierge/keeper/chain_info_indexer.go

+		// chain info has not been initialised yet
+		return err


Suggested change

// chain info has not been initialised yet

return err

return fmt.Errorf("failed to get chain info of %s: %w", chainID, err)

gitferry · 2023-01-23T14:31:26Z

x/zoneconcierge/keeper/epoch_chain_info_indexer.go

-	chainInfo := k.GetChainInfo(ctx, chainID)
+	chainInfo, err := k.GetChainInfo(ctx, chainID)
+	if err != nil {
+		// chain info does not exist yet, nothing to record


Would it be better if we add a log here? Otherwise, we would loose this information

gitferry · 2023-01-23T14:37:19Z

x/zoneconcierge/keeper/hooks.go

+			// chain info does not exist yet, initialise chain info for this chain
+			chainInfo, err = h.k.InitChainInfo(ctx, indexedHeader.ChainId)
+			if err != nil {
+				panic(err)


Suggested change

panic(err)

panic(fmt.Errorf("failed to initialize chain info of %s: %w", indexedHeader.ChainId, err)

gitferry · 2023-01-23T14:38:05Z

x/zoneconcierge/keeper/hooks.go

+				panic(err)
+			}
+		} else {
+			panic(err)


Suggested change

panic(err)

panic(fmt.Errorf("failed to get chain info of %s: %w", indexedHeader.ChainId, err)

init

a0a4155

SebastianElvis requested review from vitsalis and gusin13 January 17, 2023 22:29

gusin13 approved these changes Jan 17, 2023

View reviewed changes

vitsalis reviewed Jan 18, 2023

View reviewed changes

SebastianElvis added 2 commits January 23, 2023 14:51

Merge branch 'dev' into zoneconcierge-chaininfo-err

b2dc12c

f

1072d99

gitferry approved these changes Jan 23, 2023

View reviewed changes

fix comments

de0b373

SebastianElvis merged commit 42e3744 into dev Jan 24, 2023

SebastianElvis deleted the zoneconcierge-chaininfo-err branch January 24, 2023 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zoneconcierge: proper initialisation for chain info #285

zoneconcierge: proper initialisation for chain info #285

SebastianElvis commented Jan 17, 2023

vitsalis left a comment

vitsalis Jan 18, 2023

vitsalis Jan 18, 2023

SebastianElvis Jan 23, 2023 •

edited

Loading

vitsalis Jan 18, 2023

SebastianElvis Jan 23, 2023

vitsalis Jan 18, 2023

SebastianElvis Jan 23, 2023

vitsalis Jan 18, 2023

SebastianElvis Jan 23, 2023

gitferry Jan 23, 2023

SebastianElvis Jan 24, 2023

vitsalis Jan 18, 2023

SebastianElvis Jan 23, 2023

SebastianElvis commented Jan 23, 2023

gitferry left a comment •

edited

Loading

gitferry Jan 23, 2023

gitferry Jan 23, 2023

gitferry Jan 23, 2023

gitferry Jan 23, 2023

gitferry Jan 23, 2023

	// chain info has not been initialised yet
	return err
	return fmt.Errorf("failed to get chain info of %s: %w", chainID, err)

	panic(err)
	panic(fmt.Errorf("failed to initialize chain info of %s: %w", indexedHeader.ChainId, err)

	panic(err)
	panic(fmt.Errorf("failed to get chain info of %s: %w", indexedHeader.ChainId, err)

zoneconcierge: proper initialisation for chain info #285

zoneconcierge: proper initialisation for chain info #285

Conversation

SebastianElvis commented Jan 17, 2023

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis Jan 23, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis commented Jan 23, 2023

gitferry left a comment • 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

SebastianElvis Jan 23, 2023 •

edited

Loading

gitferry left a comment •

edited

Loading