Skip to content

Commit

Permalink
No longer disable dynamic dep generation during ACL dependency genera…
Browse files Browse the repository at this point in the history
…tion (#404)

## Describe your changes and provide context
This removes the behavior of disabling a dynamic dependency generator if
it returns something invalid during message dependency generation. This
needs to be removed so that the state writes are consistent with
synchronous execution, such that message dependency generation is a
read-only path. This change won't cause correctness issues, but in the
case that there is an incorrect dynamic dependency generator, it will
result in wasted compute since it will try it every time instead of
disabling it after the first time it was incorrect. This seems like an
acceptable compromise in order to have consistency with synchronous
execution.

## Testing performed to validate your change
Updated unit tests, and tested this behavior on a loadtest cluster
  • Loading branch information
udpatil authored Jan 25, 2024
1 parent f01454c commit 5a1afb8
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 8 deletions.
5 changes: 0 additions & 5 deletions x/accesscontrol/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -578,11 +578,6 @@ func (k Keeper) GetMessageDependencies(ctx sdk.Context, msg sdk.Msg) []acltypes.
ctx.Logger().Error(errorMessage)
}
}
if dependencyMapping.DynamicEnabled {
// there was an issue with dynamic generation, so lets disable it
// this will not error, the validation check was done in previous calls already
_ = k.SetDependencyMappingDynamicFlag(ctx, messageKey, false)
}
return dependencyMapping.AccessOps
}

Expand Down
7 changes: 4 additions & 3 deletions x/accesscontrol/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ func TestInvalidGetMessageDependencies(t *testing.T) {
delete(app.AccessControlKeeper.MessageDependencyGeneratorMapper, undelegateKey)
accessOps := app.AccessControlKeeper.GetMessageDependencies(ctx, &stakingUndelegate)
require.Equal(t, types.SynchronousMessageDependencyMapping("").AccessOps, accessOps)
require.False(t, app.AccessControlKeeper.GetResourceDependencyMapping(ctx, undelegateKey).DynamicEnabled)
// no longer gets disabled such that there arent writes in the dependency generation path
require.True(t, app.AccessControlKeeper.GetResourceDependencyMapping(ctx, undelegateKey).DynamicEnabled)
}

func TestWasmDependencyMapping(t *testing.T) {
Expand Down Expand Up @@ -2464,14 +2465,14 @@ func (suite *KeeperTestSuite) TestMessageDependencies() {
req.Equal(delegateStaticMapping.AccessOps, accessOps)
// verify dynamic got disabled
dependencyMapping = app.AccessControlKeeper.GetResourceDependencyMapping(ctx, delegateKey)
req.Equal(false, dependencyMapping.DynamicEnabled)
req.Equal(true, dependencyMapping.DynamicEnabled)

// lets also try with undelegate, but this time there is no dynamic generator, so we disable it as well
accessOps = app.AccessControlKeeper.GetMessageDependencies(ctx, &stakingUndelegate)
req.Equal(undelegateStaticMapping.AccessOps, accessOps)
// verify dynamic got disabled
dependencyMapping = app.AccessControlKeeper.GetResourceDependencyMapping(ctx, undelegateKey)
req.Equal(false, dependencyMapping.DynamicEnabled)
req.Equal(true, dependencyMapping.DynamicEnabled)
}

func (suite *KeeperTestSuite) TestImportContractReferences() {
Expand Down

0 comments on commit 5a1afb8

Please sign in to comment.