Conversation
* gmp tests * lint * add codec test * add genesis test * ignore e2e codecov * add tests * add module tests * +test * +test * +tests * +tests * +tests * +tests and bugfix * more tests * add query e2e tests * + test * +genesis tests * update * update * loop * update test * rework tests
* PacketDataUnmarshaler and PacketDataProvider for GMP * lint * shorten comment * imp: remove callback key * extend tests * bad merge fix * recursive codecov ignore * simplify codecov ignore --------- Co-authored-by: srdtrk <srdtrk@hotmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| existingAcc := k.accountKeeper.GetAccount(ctx, newAddr) | ||
| if existingAcc != nil { | ||
| // TODO: ensure this cannot be abused (refactor after this is resolved) |
There was a problem hiding this comment.
Do we need to take care of this TODO before merging?
| } | ||
|
|
||
| func (*IBCModule) OnTimeoutPacket(_ sdk.Context, _, _ string, _ uint64, _ channeltypesv2.Payload, _ sdk.AccAddress) error { | ||
| return nil |
There was a problem hiding this comment.
@srdtrk Do we intentionally do nothing in the callbacks?
In my WIP implementation of x/ift I'm currently using IBC middleware to intercept all GMP callbacks and I'm filtering out these that are not for IFT. Is it good enough for now?
There was a problem hiding this comment.
I think it is good enough. By IBC middleware, do you mean the callbacks middleware? There is no good way to make callbacks here anyway
There was a problem hiding this comment.
ok, thank you!
yes, I mean https://github.com/cosmos/ibc-go/blob/main/modules/apps/callbacks/v2/ibc_middleware.go#L60
technicallyty
left a comment
technicallyty
left a comment
There was a problem hiding this comment.
first pass over this just got some nits. is there a doc to learn about the goals/arch of gmp?
| return nil, err | ||
| } | ||
|
|
||
| k.Logger(ctx).Info("IBC send GMP packet", "sender", msg.Sender, "receiver", msg.Receiver) |
There was a problem hiding this comment.
There are a few logs, such as this one here, that seem better off under debug
There was a problem hiding this comment.
Makes sense but in other IBC apps, they also use info events. So this is more of a consistency thing
| } | ||
|
|
||
| newAcc := k.accountKeeper.NewAccountWithAddress(ctx, newAddr) | ||
| k.accountKeeper.SetAccount(ctx, newAcc) |
There was a problem hiding this comment.
in x/feegrant we make checks that the new account is not part of x/bank's blocked addresses before creating accounts w/ the account keeper. do we need to do this here?
There was a problem hiding this comment.
BankKeeper isn't passed to this module at the moment. So I think this is not needed
| runtime.NewKVStoreService(keys[gmptypes.StoreKey]), | ||
| app.AccountKeeper, | ||
| app.MsgServiceRouter(), | ||
| govAuthority, |
There was a problem hiding this comment.
what do we need authority for? i didn't see any usage in the keeper
There was a problem hiding this comment.
We don't need it but I think all other IBC apps take govAuthority. So it is there for consistency.
7 participants
Description
closes: STACK-2040
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/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.