feat: btclightclient: Refactor keepers based on needs of btcheckpoint module #72
Conversation
| // Returns false if the parent and the child are the same header. | ||
| func (k Keeper) IsAncestor(ctx sdk.Context, parentHashBytes *bbl.BTCHeaderHashBytes, childHashBytes *bbl.BTCHeaderHashBytes) (bool, error) { | ||
| // nil checks | ||
| if parentHashBytes == nil || childHashBytes == nil { |
There was a problem hiding this comment.
One comment to arguments, why using passing by pointer not by value ? In general using pointer should be used when passing larger structs to avoid copying or when arguments are optional. Imo both are not true here.
There was a problem hiding this comment.
I do this mostly out of convenience. Due to the usage of the customtype attribute on the proto files, most elements are pointers to BTCHeaderBytes or BTCHeaderHashBytes objects. If the parameters are not passed as pointers, then we would have a bunch of pointer dereferences in various places and extra checks, so I prefer to delegate those checks to when the elements are actually getting used.
There was a problem hiding this comment.
so I understand this convenience argument although not fully agree with it as:
- using
IsAncestorwithnilhashes does not really have any semantic meaning. IsAncestordoes not have full context of the caller ifnilpointer is really error or rather a panic for example, when receiving input form external source like lets say json rpc this is really an error , but retrieving this hash from our local database it should rather be panic as we should not save nil stuf in our database. (unelss of course we use pointer to represent optional value)
Nevertheless as was merged first I will adapt my pr to use pointers, so that interfaces will allign,
x/btclightclient/keeper/keeper.go
Outdated
| // If they have the same height, then the result is false | ||
| if childHeader.Height == parentHeader.Height { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Just curious why you don't consider this to be an error as well, or the why isn't the previous not an error if this one isn't.
It sounds like either the caller should know that the block behind parentHashBytes can be an ancestor of the one behind childHashBytes, or they don't. If they know that it should be okay to call this because the height is <, then they should also know not to call when the height is ==. OTOH if they don't know, then both should not be considered errors, just plain false.
| if err != nil { | ||
| t.Errorf("Valid input led to an error") | ||
| } | ||
| if isAncestor != tree.IsOnNodeChain(header, ancestor) { // The result should be whether it is an ancestor or not |
There was a problem hiding this comment.
For completeness sake I will say it it again: instead of having an algorithm to check, you should be able to generate data knowing what the outcome can be by picking a random ancestor and extending with another tree. Then you can pick any node in the extension and it's a descendant. The only question is is whether you can easily pick a node that doesn't have other descendants in the original tree.
But if you want to keep it this way that's fine as well.
There was a problem hiding this comment.
Overall, I agree. In this case, we can get an ancestor or a descendant through the usage of the tree.RandomAncestor() and tree.RandomDescendant() methods, so extra tree building is not required.
However, in this test I follow this approach in order to get two birds with one stone. Specifically, I want to test that the function returns true if the node is an anecstor and false otherwise. Randomness ensures that both the test cases are going to be tested. I don't see any difference on using the IsOnNodeChain compared to using the RandomAncestor() or the RandomDescendant functions that are used extensivelly throughout the tests and testing the two cases respectively.
This PR directly relates to the changes introduced in #70 . More specifically, we refactor some of the keepers in order to have the expected parameters and add new keepers with the requested functionality. New tests are introduced and refactored accordingly.
@KonradStaniec please verify whether this is the expected behavior. There are no major updates in the
MainChainDepthmethod since this matter is still under discussion at #70 (see discussion). Once this is resolved, this will be finalized as well.