refactor: writing test case for module account incentivizing packet #1397
Conversation
seantking
requested review from
colin-axner,
AdityaSripal,
crodriguezvega,
charleenfei and
damiannolan
as code owners
seantking
changed the title
| @@ -106,6 +108,15 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { | |||
| }, | |||
| true, | |||
| }, | |||
| { | |||
| "refund account is module account", | |||
There was a problem hiding this comment.
looking at it now, it might make more sense to put this in the escrow test suite
There was a problem hiding this comment.
it'd be nice if there was a test for distributing to a module acc
There was a problem hiding this comment.
This is great that it works out of the box... thank you, @seantking!
I guess it will also be fine, but for completeness... should we also add a test that exercises the refunding of the fees back to the module account that incentivized the packet.
| @@ -106,6 +108,15 @@ func (suite *KeeperTestSuite) TestPayPacketFee() { | |||
| }, | |||
| true, | |||
| }, | |||
| { | |||
| "refund account is module account", | |||
There was a problem hiding this comment.
it'd be nice if there was a test for distributing to a module acc
…refunding to module acc
| @@ -648,6 +650,12 @@ func (app *SimApp) LoadHeight(height int64) error { | |||
| func (app *SimApp) ModuleAccountAddrs() map[string]bool { | |||
| modAccAddrs := make(map[string]bool) | |||
| for acc := range maccPerms { | |||
| // do not add mock module to blocked addresses | |||
There was a problem hiding this comment.
This is required so that we can use mock module as a non blocked module account addr
There was a problem hiding this comment.
Nice we should add a changelog for this!
| { | ||
| "refund account is module account", | ||
| func() { | ||
| msg.Signer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(disttypes.ModuleName).String() |
There was a problem hiding this comment.
I think we can maybe remove this test case now?
There was a problem hiding this comment.
Is it worth keeping this in, and just change disttypes.ModuleName to mock.ModuleName?
There was a problem hiding this comment.
I agree. It's worth keeping it.
| @@ -648,6 +650,12 @@ func (app *SimApp) LoadHeight(height int64) error { | |||
| func (app *SimApp) ModuleAccountAddrs() map[string]bool { | |||
| modAccAddrs := make(map[string]bool) | |||
| for acc := range maccPerms { | |||
| // do not add mock module to blocked addresses | |||
There was a problem hiding this comment.
Nice we should add a changelog for this!
| { | ||
| "refund account is module account", | ||
| func() { | ||
| msg.Signer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(disttypes.ModuleName).String() |
There was a problem hiding this comment.
Is it worth keeping this in, and just change disttypes.ModuleName to mock.ModuleName?
There was a problem hiding this comment.
Thanks for all the detective work with blocked addresses to get this to work, @seantking! 🔎
| { | ||
| "refund account is module account", | ||
| func() { | ||
| msg.Signer = suite.chainA.GetSimApp().AccountKeeper.GetModuleAddress(disttypes.ModuleName).String() |
There was a problem hiding this comment.
I agree. It's worth keeping it.
Description
closes: #1329
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