Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add oracle flag & flag checker to msgupdateoracle of opchild #105

Merged
merged 4 commits into from
Aug 28, 2024
Merged

add oracle flag & flag checker to msgupdateoracle of opchild #105

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b25e356
add oracle flag & flag checker to msgupdateoracle of opchild
sh-cha Aug 27, 2024
b4f55c8
update launch cmd
beer-1 Aug 28, 2024
295e67b
fix reflect ptr
beer-1 Aug 28, 2024
f4ecc01
add more prompt
beer-1 Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,304 changes: 1,154 additions & 150 deletions api/opinit/ophost/v1/tx.pulsar.go
View file
Open in desktop

Large diffs are not rendered by default.

181 changes: 123 additions & 58 deletions api/opinit/ophost/v1/types.pulsar.go
View file
Open in desktop

Large diffs are not rendered by default.

17 changes: 16 additions & 1 deletion proto/opinit/ophost/v1/tx.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ message MsgUpdateBatchInfo {
option (amino.name) = "ophost/MsgUpdateBatchInfo";

// authority is the address that controls the module (defaults to x/gov unless overwritten)
// or the current challenger address.
// or the current proposer address.
string authority = 1 [(gogoproto.moretags) = "yaml:\"authority\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
uint64 bridge_id = 2 [(gogoproto.moretags) = "yaml:\"bridge_id\""];
BatchInfo new_batch_info = 3
Expand All @@ -263,6 +263,21 @@ message MsgUpdateBatchInfoResponse {
uint64 l2_block_number = 2;
}

// MsgUpdateOracleFlag is a message to change oracle config
message MsgUpdateOracleConfig {
option (cosmos.msg.v1.signer) = "authority";
option (amino.name) = "ophost/MsgUpdateOracleConfig";

// authority is the address that controls the module (defaults to x/gov unless overwritten)
// or the current proposer address.
string authority = 1 [(gogoproto.moretags) = "yaml:\"authority\"", (cosmos_proto.scalar) = "cosmos.AddressString"];
uint64 bridge_id = 2 [(gogoproto.moretags) = "yaml:\"bridge_id\""];
bool oracle_enabled = 3 [(gogoproto.moretags) = "yaml:\"oracle_enabled\""];
}

// MsgUpdateOracleFlagResponse returns a message handle result.
message MsgUpdateOracleConfigResponse {}

// MsgUpdateMetadata is a message to change metadata
message MsgUpdateMetadata {
option (cosmos.msg.v1.signer) = "authority";
Expand Down
6 changes: 5 additions & 1 deletion proto/opinit/ophost/v1/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,12 @@ message BridgeConfig {
];
// The the first l2 block will be recorded on l1.
uint64 submission_start_height = 6;

// oracle_enabled is a flag to enable oracle.
bool oracle_enabled = 7;

// Normally it is IBC channelID for permissioned IBC relayer.
bytes metadata = 7;
bytes metadata = 8;
}

// BatchInfo defines the set of batch information.
Expand Down
11 changes: 10 additions & 1 deletion x/opchild/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,16 @@
return nil, err
}

err := ms.Keeper.ApplyOracleUpdate(ctx, req.Height, req.Data)
// config check
info, err := ms.Keeper.BridgeInfo.Get(ctx)
if err != nil {
return nil, err
}

Check warning on line 596 in x/opchild/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/opchild/keeper/msg_server.go#L595-L596 

Added lines #L595 - L596 were not covered by tests
if !info.BridgeConfig.OracleEnabled {
return nil, types.ErrOracleDisabled
}
Comment on lines +592 to +599
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! But add tests for the configuration check.

The added configuration check enhances the robustness of the function. However, the added lines are not covered by tests.

The code changes are approved.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 595-596: x/opchild/keeper/msg_server.go#L595-L596
Added lines #L595 - L596 were not covered by tests


err = ms.Keeper.ApplyOracleUpdate(ctx, req.Height, req.Data)
if err != nil {
return nil, err
}
Expand Down
24 changes: 24 additions & 0 deletions x/opchild/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,9 @@ func Test_MsgServer_UpdateOracle(t *testing.T) {
bridgeInfo := types.BridgeInfo{
L1ChainId: defaultHostChainId,
L1ClientId: defaultClientId,
BridgeConfig: ophosttypes.BridgeConfig{
OracleEnabled: true,
},
}
err := opchildKeeper.BridgeInfo.Set(ctx, bridgeInfo)
require.NoError(t, err)
Expand Down Expand Up @@ -621,3 +624,24 @@ func Test_MsgServer_UpdateOracle(t *testing.T) {
_, err = ms.UpdateOracle(ctx, types.NewMsgUpdateOracle(addrsStr[1], 11, extCommitBz))
require.Error(t, err)
}

func Test_MsgServer_UpdateOracleFail(t *testing.T) {
ctx, input := createDefaultTestInput(t)
opchildKeeper := input.OPChildKeeper

defaultHostChainId := "test-host-1"
defaultClientId := "test-client-id"
bridgeInfo := types.BridgeInfo{
L1ChainId: defaultHostChainId,
L1ClientId: defaultClientId,
BridgeConfig: ophosttypes.BridgeConfig{
OracleEnabled: false,
},
}
err := opchildKeeper.BridgeInfo.Set(ctx, bridgeInfo)
require.NoError(t, err)

ms := keeper.NewMsgServerImpl(opchildKeeper)
_, err = ms.UpdateOracle(ctx, types.NewMsgUpdateOracle(addrsStr[0], 11, []byte{}))
require.EqualError(t, err, types.ErrOracleDisabled.Error())
}
3 changes: 2 additions & 1 deletion x/opchild/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ var (
ErrOracleTimestampNotExists = errorsmod.Register(ModuleName, 25, "oracle timestamp does not exist")
ErrInvalidOracleTimestamp = errorsmod.Register(ModuleName, 26, "oracle timestamp is old")
ErrBridgeInfoNotExists = errorsmod.Register(ModuleName, 27, "bridge info does not exist")
ErrOracleDisabled = errorsmod.Register(ModuleName, 28, "oracle is disabled")

// Antehandler error
ErrRedundantTx = errorsmod.Register(ModuleName, 28, "tx messages are all redundant")
ErrRedundantTx = errorsmod.Register(ModuleName, 29, "tx messages are all redundant")
)
31 changes: 31 additions & 0 deletions x/ophost/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
sdk.NewAttribute(types.AttributeKeyBatchChainType, req.Config.BatchInfo.ChainType.StringWithoutPrefix()),
sdk.NewAttribute(types.AttributeKeyBatchSubmitter, req.Config.BatchInfo.Submitter),
sdk.NewAttribute(types.AttributeKeyBridgeId, strconv.FormatUint(bridgeId, 10)),
sdk.NewAttribute(types.AttributeKeyOracleEnabled, strconv.FormatBool(req.Config.OracleEnabled)),
))

if err := ms.bridgeHook.BridgeCreated(ctx, bridgeId, req.Config); err != nil {
Expand Down Expand Up @@ -497,6 +498,36 @@
}, nil
}

func (ms MsgServer) UpdateOracleConfig(ctx context.Context, req *types.MsgUpdateOracleConfig) (*types.MsgUpdateOracleConfigResponse, error) {
if err := req.Validate(ms.authKeeper.AddressCodec()); err != nil {
return nil, err
}

Check warning on line 504 in x/ophost/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/ophost/keeper/msg_server.go#L503-L504 

Added lines #L503 - L504 were not covered by tests
Comment on lines +501 to +504
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure validation errors are covered by tests.

The validation check for the request is not covered by tests.

Add a test case to cover validation errors for the request.

Tools
GitHub Check: codecov/patch

[warning] 503-504: x/ophost/keeper/msg_server.go#L503-L504
Added lines #L503 - L504 were not covered by tests


bridgeId := req.BridgeId
config, err := ms.GetBridgeConfig(ctx, bridgeId)
if err != nil {
return nil, err
}

Check warning on line 510 in x/ophost/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/ophost/keeper/msg_server.go#L509-L510 

Added lines #L509 - L510 were not covered by tests
Comment on lines +506 to +510
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure error handling for bridge configuration retrieval is covered by tests.

The error handling for retrieving the bridge configuration is not covered by tests.

Add a test case to cover errors when retrieving the bridge configuration.

Tools
GitHub Check: codecov/patch

[warning] 509-510: x/ophost/keeper/msg_server.go#L509-L510
Added lines #L509 - L510 were not covered by tests


// gov or current proposer can update metadata.
if ms.authority != req.Authority && config.Proposer != req.Authority {
return nil, govtypes.ErrInvalidSigner.Wrapf("invalid authority; expected %s or %s, got %s", ms.authority, config.Proposer, req.Authority)
}
Comment on lines +512 to +515
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure permission checks are covered by tests.

The permission check for updating the Oracle configuration is correctly implemented but not covered by tests.

Add a test case to cover permission checks for updating the Oracle configuration.


config.OracleEnabled = req.OracleEnabled

if err := ms.SetBridgeConfig(ctx, bridgeId, config); err != nil {
return nil, err
}

Check warning on line 521 in x/ophost/keeper/msg_server.go

View check run for this annotation

Codecov / codecov/patch

x/ophost/keeper/msg_server.go#L520-L521 

Added lines #L520 - L521 were not covered by tests
Comment on lines +517 to +521
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure setting the bridge configuration is covered by tests.

The process of setting the bridge configuration is not covered by tests.

Add a test case to cover setting the bridge configuration.

Tools
GitHub Check: codecov/patch

[warning] 520-521: x/ophost/keeper/msg_server.go#L520-L521
Added lines #L520 - L521 were not covered by tests


sdk.UnwrapSDKContext(ctx).EventManager().EmitEvent(sdk.NewEvent(
types.EventTypeUpdateBatchInfo,
sdk.NewAttribute(types.AttributeKeyBridgeId, strconv.FormatUint(bridgeId, 10)),
sdk.NewAttribute(types.AttributeKeyOracleEnabled, strconv.FormatBool(config.OracleEnabled)),
))
Comment on lines +523 to +527
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure event emission is covered by tests.

The event emission for updating the Oracle configuration is correctly implemented but not covered by tests.

Add a test case to cover event emission for updating the Oracle configuration.

return &types.MsgUpdateOracleConfigResponse{}, nil
}

func (ms MsgServer) UpdateMetadata(ctx context.Context, req *types.MsgUpdateMetadata) (*types.MsgUpdateMetadataResponse, error) {
if err := req.Validate(ms.authKeeper.AddressCodec()); err != nil {
return nil, err
Expand Down
47 changes: 46 additions & 1 deletion x/ophost/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,51 @@ func Test_UpdateBatchInfo(t *testing.T) {
require.Error(t, err)
}

func Test_UpdateOracleConfig(t *testing.T) {
ctx, input := createDefaultTestInput(t)
ms := keeper.NewMsgServerImpl(input.OPHostKeeper)

config := types.BridgeConfig{
Proposer: addrsStr[0],
Challengers: []string{addrsStr[1]},
SubmissionInterval: time.Second * 10,
FinalizationPeriod: time.Second * 60,
SubmissionStartHeight: 1,
Metadata: []byte{1, 2, 3},
BatchInfo: types.BatchInfo{Submitter: addrsStr[0], ChainType: types.BatchInfo_CHAIN_TYPE_INITIA},
OracleEnabled: true,
}

_, err := ms.CreateBridge(ctx, types.NewMsgCreateBridge(addrsStr[0], config))
require.NoError(t, err)

// gov signer
govAddr, err := input.AccountKeeper.AddressCodec().BytesToString(authtypes.NewModuleAddress("gov"))
require.NoError(t, err)
msg := types.NewMsgUpdateOracleConfig(govAddr, 1, false)
_, err = ms.UpdateOracleConfig(ctx, msg)
require.NoError(t, err)
_config, err := ms.GetBridgeConfig(ctx, 1)
require.NoError(t, err)
require.Equal(t, false, _config.OracleEnabled)

// current proposer signer
msg = types.NewMsgUpdateOracleConfig(addrsStr[0], 1, true)
_, err = ms.UpdateOracleConfig(ctx, msg)
require.NoError(t, err)
_config, err = ms.GetBridgeConfig(ctx, 1)
require.NoError(t, err)
require.Equal(t, true, _config.OracleEnabled)

// invalid signer
invalidAddr, err := input.AccountKeeper.AddressCodec().BytesToString(authtypes.NewModuleAddress(types.ModuleName))
require.NoError(t, err)
msg = types.NewMsgUpdateOracleConfig(invalidAddr, 1, false)
require.NoError(t, err)

_, err = ms.UpdateOracleConfig(ctx, msg)
require.Error(t, err)
}
func Test_UpdateMetadata(t *testing.T) {
ctx, input := createDefaultTestInput(t)
ms := keeper.NewMsgServerImpl(input.OPHostKeeper)
Expand Down Expand Up @@ -469,7 +514,7 @@ func Test_UpdateMetadata(t *testing.T) {
require.Equal(t, []byte{4, 5, 6}, _config.Metadata)
require.Equal(t, []byte{4, 5, 6}, input.BridgeHook.metadata)

// current challenger
// current proposer
msg = types.NewMsgUpdateMetadata(addrsStr[0], 1, []byte{7, 8, 9})
_, err = ms.UpdateMetadata(ctx, msg)
require.NoError(t, err)
Expand Down
1 change: 1 addition & 0 deletions x/ophost/types/event.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,5 @@ const (
AttributeKeyL2Sequence = "l2_sequence"
AttributeKeyFinalizedOutputIndex = "finalized_output_index"
AttributeKeyFinalizedL2BlockNumber = "finalized_l2_block_number"
AttributeKeyOracleEnabled = "oracle_enabled"
)
27 changes: 27 additions & 0 deletions x/ophost/types/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,33 @@
return nil
}

/* MsgUpdateOracleConfig */

// NewMsgUpdateOracleConfig creates a new MsgUpdateOracleConfig instance.
func NewMsgUpdateOracleConfig(
authority string,
bridgeId uint64,
oracleEnabled bool,
) *MsgUpdateOracleConfig {
return &MsgUpdateOracleConfig{
Authority: authority,
BridgeId: bridgeId,
OracleEnabled: oracleEnabled,
}

Check warning on line 395 in x/ophost/types/tx.go

View check run for this annotation

Codecov / codecov/patch

x/ophost/types/tx.go#L390-L395 

Added lines #L390 - L395 were not covered by tests
}

// Validate performs basic MsgUpdateOracleConfig message validation.
func (msg MsgUpdateOracleConfig) Validate(accAddressCodec address.Codec) error {
if _, err := accAddressCodec.StringToBytes(msg.Authority); err != nil {
return err
}

Check warning on line 402 in x/ophost/types/tx.go

View check run for this annotation

Codecov / codecov/patch

x/ophost/types/tx.go#L399-L402 

Added lines #L399 - L402 were not covered by tests

if msg.BridgeId == 0 {
return ErrInvalidBridgeId
}
return nil

Check warning on line 407 in x/ophost/types/tx.go

View check run for this annotation

Codecov / codecov/patch

x/ophost/types/tx.go#L404-L407 

Added lines #L404 - L407 were not covered by tests
Comment on lines +398 to +407
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! But add tests for validation.

The validation logic is correct. However, the added lines are not covered by tests.

The code changes are approved.

Do you want me to generate the unit testing code or open a GitHub issue to track this task?

Tools
GitHub Check: codecov/patch

[warning] 399-402: x/ophost/types/tx.go#L399-L402
Added lines #L399 - L402 were not covered by tests

[warning] 404-407: x/ophost/types/tx.go#L404-L407
Added lines #L404 - L407 were not covered by tests

}

/* MsgUpdateMetadata */

// NewMsgUpdateMetadata creates a new MsgUpdateMetadata instance.
Expand Down
Loading
Loading