IsFrozen() changed to Status() #140
Conversation
|
Marking r4r, just need to add gRPC + gRPC test |
Codecov Report
@@ Coverage Diff @@
## main #140 +/- ##
===========================================
+ Coverage 65.92% 79.17% +13.24%
===========================================
Files 131 109 -22
Lines 8382 6526 -1856
===========================================
- Hits 5526 5167 -359
+ Misses 2476 1004 -1472
+ Partials 380 355 -25
|
| // Expired is a potential status of a client. A client is expired | ||
| // if its latest consensus state timestamp added to its trusting period | ||
| // is before or equal to the current time. | ||
| const Expired = "Expired" |
There was a problem hiding this comment.
nit, this should be defined in exported too?
There was a problem hiding this comment.
My reasoning is that this is a tendermint specific concept. Is expiry common amongst light clients? cc @AdityaSripal @cwgoes
There was a problem hiding this comment.
Expiry will be common to light clients other than Tendermint, yes. Anything for proof-of-stake should have this.
modules/core/exported/client.go
Outdated
| // Active is a status type of a client. An active client is allowed to be used. | ||
| Active string = "Active" | ||
|
|
||
| // Frozen is a status type of a client. A frozen client is not allowed to be used. | ||
| Frozen string = "Frozen" | ||
|
|
||
| // Unknown indicates there was an error in determining the status of a client | ||
| Unknown string = "Unknown" |
There was a problem hiding this comment.
should these be proto enums?
There was a problem hiding this comment.
No? They aren't used in communication. So I think it is probably simplest to leave them as strings. (only use proto when necessary)
| {"req is nil", | ||
| func() { | ||
| req = nil | ||
| }, | ||
| false, | ||
| }, |
There was a problem hiding this comment.
this test case panics using the old old testing setup. Another reason to use the new one 🙂
modules/light-clients/06-solomachine/types/client_state_test.go
Outdated
Show resolved
Hide resolved
…o colin/98-client-status
| consState, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.GetClientConsensusState(suite.chainA.GetContext(), path.EndpointA.ClientID, trustedHeight) | ||
| suite.Require().True(found) |
There was a problem hiding this comment.
Why don't you have this check in future function?
There was a problem hiding this comment.
I added this because the consensus state is required to know the correct timestamp. The future function doesn't need to pass in a timestamp and thus the consensus state isn't needed. Checking for the existence of the consensus state would break a test case below, I'm fairly sure - "consensus state not found"
modules/core/exported/client.go
Outdated
| Active string = "Active" | ||
|
|
||
| // Frozen is a status type of a client. A frozen client is not allowed to be used. | ||
| Frozen string = "Frozen" | ||
|
|
||
| // Unknown indicates there was an error in determining the status of a client | ||
| Unknown string = "Unknown" |
There was a problem hiding this comment.
These should all be golang enums at the very least. Otherwise there is no obvious relationship between them, and the type of Status is string rather than exported.Status
There was a problem hiding this comment.
Can you elaborate on your concern of using strings? My primary concern of using enums is modularity and value registration
Enums are useful when you know the full range of constants (connection/channel state is a good example) since it provides uniqueness. We cannot know all types of status a client may have. The problem is that every light client developer would need to register their own enums in order to avoid registration conflict. Registering status types seems unnecessarily complicated without a concrete benefit. I think the performance impact of using constants should be pretty low (if any impact).
It would also make maintaining backwards compatibility difficult since we would either need to pre-reserve a selected amount of enums (otherwise we could never make a backwards compatible addition)
I tried using a middle ground solution of using a typed constant
type Status string
This changes the function signature to use exported.Status instead of string, but it makes using the gRPC clunky since we cannot use exported.Status as a return type in the gRPC route. Thus we need to cast to/from the exported.Status when returning. Relayers and external third parties would rely on string (unless they decided to cast) whereas the code would rely on its typed string.
I think my preference is continuing to use string. Client types are strings and we use those in identifiers. I don't see the benefit in increasing complexity and potentially creating a divide between what type is used in core IBC vs outside core IBC.
Thoughts?
There was a problem hiding this comment.
Thinking more on this, I guess I'm ok with strings.
My concern is there isn't a very clear global view on what a Status means, beyond just Active or some other value.
At the code level this is really just acting as a boolean.
I would prefer just an enum: Active or Inactive. And either log the specific issue for inactiveness, or allow it as a specific query on the client state.
But in the absence of that, strings are fine
There was a problem hiding this comment.
I agree that "Active" is arbitrary and is likely something developers are capable of missing. Same with "Frozen"
The primary benefit for Active to be used not to be used as a boolean is trivial support for a gRPC route, as stated in the linked issue:
The benefit of using Status() as opposed to IsActive() is trivial support for a Status gRPC route. If the client state interface only supported IsActive then we would likely need to add a Status gRPC route for every light client as opposed to defining it once in 02-client.
What about this as the function signature:
Status(ctx, clientStore, cdc) exported.Status, string
Where exported.Status is an enum of either Active, Frozen, Inactive and string is any additional information such as "Expired"?
Core IBC only uses the string in error messages and for gRPC support
There was a problem hiding this comment.
I like this idea! Though I think we just need Active, Inactive no? Frozen is a particular type of inactive
There was a problem hiding this comment.
I like this idea! Though I think we just need
Active, Inactiveno? Frozen is a particular type of inactive
I believe #141 requires knowing if the client is Frozen? That's the only use case that comes to mind. No strong preference on this decision. Happy to do whichever you think is best
There was a problem hiding this comment.
Going with Active, Frozen, Inactive since misbehaviour/freezing is already a concept in core IBC, it makes sense to have this status type
There was a problem hiding this comment.
I decided to use a typed string. Initially I implemented a typed int32 and the function signature above (hence the force push), but I realized separating the status and info felt unnecessary. The main critique in this thread is separating a string from a status and a typed string accomplishes that.
It also is self descriptive. The value 1 indicates Frozen a lot less than the string "Frozen". Furthermore, backwards compatibility will be easier since the next value added wouldn't have to be 3.
Since we are using a typed string, I kept Active, Frozen, Expired, Unknown as the default supported status types. Core IBC should only ever look for Active and potentially Frozen in exceptional cases.
I tried using proto enums but proto enums will just create a typed int32 and requires the active status to be suffixed with _UNSPECIFIED per proto linting. In the gRPC response, since I couldn't use a typed Status, I went with string as I figured relayers would prefer to compare against a self descriptive string rather than an arbitrary number.
I think there are also less risks for errors, since core IBC defensively uses the status (must be Active to do anything). If I used a typed int32, a typed status return value of 0 would always be interpreted as Active by core IBC. For example
var inactive exported.Status = 0
return inactive
With a typed string, returning a typed status of "Active" is self descriptive. You wouldn't use that return value to indicate the client is inactive.
This last part is a minor nit, but a typed int32 just felt like unnecessary indirection.
I don't understand the usefulness of this. If you don't call In the tendermint case, you can almost never rely on a non-refreshed status as time is always incrementing (except for during the execution of a block) I'm going to leave the API as is. I think allowing stale status' runs a higher risk of bugs as it would be problematic if we relied on a non-refreshed status. If the block execution always relies on refreshing the status, then we gain no benefit from separating the logic and increasing the overhead on light client implementations. |
|
My concern is if I want to make a Status check, I need to have My proposal was that you call |
|
I agree The primary motivation for this change was based on 2 issues
There are a few constraints:
There's no way around the first constraint, thus the question is whether we should cache the status Trying to cache the status in the client state causes us to deal with backwards compatibility. I believe it isn't possible to do a backwards compatible fix without introducing client versions. Client versions are useful regardless, but that fix is out of scope of this issue. Furthermore, a modification to the client state proto file should really require an ICS spec change gRPC callers need to set the current time in the context. This might be difficult for non-go implementations. I'm not sure if it is possible for relayers like Lets say we wanted to cache the status. We know that because the context is passed in, we can probably assume each status is tied to a block height (of the current chain, note this block height is not the same as the consensus state block height as they represent different chains). Thus we could create a mapping from block height -> status. This doesn't solve the gRPC constraint since queries at a height which didn't have a status set would need to depend on the context block time which may or may not be set. If a status isn't set for a block height, it is not safe to assume it has the same status as the next/prev block height which has a status set. Using the client store mapping, a helper function would still need access to the client store and ctx (and potentially cdc) Based on the above reasoning, without any specific use case in mind for caching the status, I'd prefer to do the simplest solution possible. Otherwise we increase the amount of things all light clients would need to do or break IBC. I think the heftiness of I like the naming of I'm not really sure how you could cache the status within the client state since the status is dependent upon the block height it was updated against. Therefore a client status would need to keep its current status and the block height it was updated against, otherwise it is impossible to verify if the status is stale
My main concern is there is no clear contract here. The |
colin-axner
force-pushed
the
colin/98-client-status
branch
from
17efb17 to
cca717d
Compare
Add integration tests with updated wasm contracts
Description
closes: #98
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes