Store historic header information for IBC #93
Conversation
alpe
force-pushed
the
82-historic-validator-information
branch
from
133dbfb
to
f22295b
Compare
alpe
force-pushed
the
82-historic-validator-information
branch
from
f22295b
to
2834fbe
Compare
| @@ -269,7 +269,7 @@ func NewTgradeApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest | |||
|
|
|||
| // Create IBC Keeper | |||
| app.ibcKeeper = ibckeeper.NewKeeper( | |||
| appCodec, keys[ibchost.StoreKey], app.getSubspace(ibchost.ModuleName), stakingKeeper, scopedIBCKeeper, | |||
| appCodec, keys[ibchost.StoreKey], app.getSubspace(ibchost.ModuleName), &app.poeKeeper, scopedIBCKeeper, | |||
There was a problem hiding this comment.
Ah.. and now it has state once again 😄
But... how can app.poeKeeper be used on line 272 when it is set in 352? This smells funny
There was a problem hiding this comment.
A ref is parsed to the ibc keeper that is updated later. I will add a test that provides more confidence
There was a problem hiding this comment.
A ref is passed to app.poeKeeper (probably the zero value).
Later we set app.poeKeeper = &stakingtypes.Adaptor{} or something like that, which does not update the other reference.
If you did *app.poeKeeper = staking types.Adaptor{}, then this would update the data that was pointed at by the same reference app.ibcKeeper has.
At least this is my understanding of pointers. As it is now, it points into space. This is also a bit harder to reason about when most of these are just empty structs, and return errors on all calls... when there is real state and logic, it will be most difficult
There was a problem hiding this comment.
Please note app.poeKeeper is of type poekeeper.Keeper not a pointer.
When I pass the ref here it points to the empty (not nil) app.poeKeeper memory address. When the poeKeeper is instantiated, it is still the same memory address.
https://play.golang.org/p/y9VGNrLUUVC
There was a problem hiding this comment.
I need to look into this...
There was a problem hiding this comment.
That didn't quite convince me, so I played with it a bit more, to replicate more precisely how we use pointers: https://play.golang.org/p/xBlHLRh_ye_K
And that still matches your expected behaviour.
I guess I really don't understand how all these pointers and interfaces are implemented under the hood in Go, but your code is correct in spite of my confusion.
| height := ibcclienttypes.Height{RevisionNumber: 1, RevisionHeight: 2} | ||
|
|
||
| // when | ||
| state, found := gapp.ibcKeeper.ClientKeeper.GetSelfConsensusState(ctx, height) |
There was a problem hiding this comment.
This uses the staking adaptor? Can you link to where that call happens?
Can we assert that data that we expect to be returned from such an adaptor (unbonding period?) is actually set properly? Not just that it returns "some data"
There was a problem hiding this comment.
I have extended the test to verify that the data returned is what was persisted before on begin block
There was a problem hiding this comment.
Thank you. Looks convincing.
| height := ibcclienttypes.Height{RevisionNumber: 1, RevisionHeight: 2} | ||
|
|
||
| // when | ||
| state, found := gapp.ibcKeeper.ClientKeeper.GetSelfConsensusState(ctx, height) |
There was a problem hiding this comment.
Thank you. Looks convincing.
Resolves #82
After #91
The
historical_information.gowas copied from cosmos-sdk with small modifications. Credits and big thanks to the original authorsImplements the IBC
StakingKeeperinterfaceNot included:
If we need any them, let's add a new issue.