feat(appchain): load subscriber genesis, ibc #176
Conversation
x/avs handles the assetID validation
- Anyone can create a subchain via a simple transaction. The genesis file is available at the end of the current epoch. - If no operators opt in by that time, it is dropped. - If the state is available, it is queried via CLI and added to the genesis file of the subchain. - The genesis file, with data from other modules, starts the chain once enough vote power is live
The AVS module now validates the assetIDs passed to it
negative duration is also not supported
The wrapped consensus key object has utility in the subscriber module as well, and, may have some utility in the coordinator module. Hence, it is better located in a `types` subpackage.
|
@coderabbitai Review ✅ Actions performedReview triggered.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
x/appchain/coordinator/keeper/slash.go (3)
12-40: LGTM: ValidateSlashPacket function looks good.The function performs necessary validations for the slashing packet, including checking the valset update ID and operator account. The error messages are descriptive and helpful.
Consider adding a brief comment explaining what "stateless validation" entails in this context, as mentioned in the function comment.
42-74: LGTM: HandleSlashPacket function is well-implemented.The function correctly handles the slashing process, retrieving necessary data and applying the slashing logic. The error handling for
ApplySlashForHeightis appropriate.Regarding the TODO comment about moving fields to the AVS keeper:
Would you like me to create a GitHub issue to track the task of moving the relevant fields to the AVS keeper?
100-100: Consider creating a task for AVS keeper refactoring.The TODO comment suggests moving certain fields to the AVS keeper. This aligns with the earlier comment in HandleSlashPacket.
Would you like me to create a GitHub issue to track the task of moving the relevant fields and functions to the AVS keeper?
utils/utils.go (1)
256-264: LGTM with suggestion: ChainIDWithoutRevision functionThe
ChainIDWithoutRevisionfunction correctly extracts the chain ID without the revision number. It properly uses theIsRevisionFormatfunction to determine if the chain ID contains a revision.However, the current implementation assumes that there's only one hyphen in the chain ID. While this works for the current format (e.g., "exocoretestnet_233-1"), it might be more robust to use
strings.LastIndexto handle potential future changes where the chain ID itself might contain hyphens.Consider updating the implementation to use
strings.LastIndex:func ChainIDWithoutRevision(chainID string) string { if !ibcclienttypes.IsRevisionFormat(chainID) { return chainID } lastHyphen := strings.LastIndex(chainID, "-") if lastHyphen == -1 { return chainID } return chainID[:lastHyphen] }This change ensures that the function will work correctly even if the chain ID format changes to include hyphens in the future.
x/appchain/subscriber/keeper/keeper.go (2)
77-79: Check for duplicate port binding inBindPortmethodWhen binding a port, it's good practice to check if the port is already bound to prevent unexpected behavior or errors.
Consider adding a check using
IsBoundbefore binding the port:func (k Keeper) BindPort(ctx sdk.Context, portID string) error { + if k.IsBound(ctx, portID) { + return fmt.Errorf("port %s is already bound", portID) + } capability := k.portKeeper.BindPort(ctx, portID) return k.ClaimCapability(ctx, capability, host.PortPath(portID)) }
53-53: Simplify the logger module name formattingThe module name formatting in the
Loggermethod can be streamlined for clarity.Apply this diff to simplify the code:
return ctx.Logger().With("module", fmt.Sprintf("x/%s", types.ModuleName)) +// Since types.ModuleName already represents "x/<module_name>" +return ctx.Logger().With("module", types.ModuleName)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/appchain/subscriber/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
- proto/exocore/appchain/subscriber/v1/genesis.proto (1 hunks)
- utils/utils.go (2 hunks)
- x/appchain/common/types/types.go (1 hunks)
- x/appchain/common/types/wire.go (1 hunks)
- x/appchain/coordinator/keeper/slash.go (1 hunks)
- x/appchain/coordinator/module_ibc.go (1 hunks)
- x/appchain/coordinator/types/expected_keepers.go (2 hunks)
- x/appchain/subscriber/keeper/connection.go (1 hunks)
- x/appchain/subscriber/keeper/keeper.go (1 hunks)
- x/appchain/subscriber/keeper/validators.go (1 hunks)
- x/appchain/subscriber/module_ibc.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- proto/exocore/appchain/subscriber/v1/genesis.proto
- x/appchain/common/types/types.go
- x/appchain/common/types/wire.go
- x/appchain/coordinator/types/expected_keepers.go
- x/appchain/subscriber/keeper/connection.go
- x/appchain/subscriber/module_ibc.go
🧰 Additional context used
📓 Learnings (3)
utils/utils.go (1)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore#176 File: utils/utils.go:259-265 Timestamp: 2024-10-13T15:31:11.016Z Learning: The `IsRevisionFormat` function enforces that chain IDs are in the format `{chainID}-{revision}`, ensuring that base chain IDs do not contain additional hyphens.x/appchain/coordinator/keeper/slash.go (3)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore#176 File: x/appchain/coordinator/keeper/slash.go:61-61 Timestamp: 2024-10-13T15:37:24.858Z Learning: In `HandleSlashPacket`, it's unnecessary to check if the AVS exists because the packet must be received over a channel belonging to a chain known to the coordinator module, which mandatorily registers the AVS.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore#176 File: x/appchain/coordinator/keeper/slash.go:56-56 Timestamp: 2024-10-13T15:27:56.082Z Learning: The `slashProportion` value is already verified at the time of registration of the subscriber chain, so error handling when parsing it in `HandleSlashPacket` is unnecessary.Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore#176 File: x/appchain/coordinator/keeper/slash.go:82-83 Timestamp: 2024-10-13T15:27:48.641Z Learning: In the Go codebase, specifically in the `GetSlashAcks` method in `x/appchain/coordinator/keeper/slash.go`, it's unnecessary to check for `nil` before unmarshalling because `k.cdc.MustUnmarshal(nil, &consAddresses)` does not cause a panic.x/appchain/coordinator/module_ibc.go (1)
Learnt from: MaxMustermann2 PR: ExocoreNetwork/exocore#176 File: x/appchain/coordinator/module_ibc.go:207-220 Timestamp: 2024-10-13T15:32:07.972Z Learning: In `x/appchain/coordinator/module_ibc.go`, within the `OnRecvPacket` function, a default case is already present to handle unexpected packet types.
🪛 GitHub Check: CodeQL
utils/utils.go
[warning] 232-234: Iteration over map
Iteration over map may be a possible source of non-determinismx/appchain/coordinator/module_ibc.go
[warning] 237-237: Impossible interface nil check
This value can never be nil, since it is a wrapped interface value.
🔇 Additional comments (20)
x/appchain/coordinator/keeper/slash.go (5)
6-7: LGTM: New imports are appropriate.The added imports for
utilsandcommontypesare relevant to the new functionality introduced in this file.
76-81: LGTM: AppendSlashAck function is correctly implemented.The function efficiently appends the new consensus address to the existing list of slashing acknowledgments.
83-91: LGTM: GetSlashAcks function is correctly implemented.The function efficiently retrieves and unmarshals the slashing acknowledgments. As per the provided learning, it's unnecessary to check for nil before unmarshalling in this context.
93-98: LGTM: SetSlashAcks function is correctly implemented.The function efficiently marshals and stores the consensus addresses for slashing acknowledgments.
Line range hint
1-100: Overall, the changes in this file are well-implemented and improve slashing functionality.The new functions for validating and handling slashing packets, as well as managing slash acknowledgments, are correctly implemented. The code is clean, follows good practices, and includes helpful comments. The TODO comments provide clear direction for future improvements, particularly regarding moving certain fields to the AVS keeper.
Great job on these additions and improvements!
utils/utils.go (2)
216-246: LGTM: AccumulateChanges function implementationThe
AccumulateChangesfunction effectively accumulates and deduplicates validator updates. The implementation ensures uniqueness using a map and determinism through sorting, which is crucial for consensus across nodes.Regarding the static analysis hint about map iteration (lines 232-234):
This is not a concern in this context because the resulting slice is sorted immediately after the iteration (lines 238-243), ensuring deterministic output.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 232-234: Iteration over map
Iteration over map may be a possible source of non-determinism
248-254: LGTM: AppendMany function implementationThe
AppendManyfunction is a concise and efficient implementation for appending multiple byte slices. It uses a variadic parameter, allowing for flexible input, and the implementation is straightforward and correct.x/appchain/subscriber/keeper/keeper.go (1)
119-130: Ensure key uniqueness inSetPacketMaturityTimeThe
types.PacketMaturityTimeKey(vscID, maturityTime)function should generate unique keys to prevent accidental overwriting of data. Ensure that the combination ofvscIDandmaturityTimeis unique across the application.To confirm the uniqueness of the keys, you can check the key generation logic:
x/appchain/coordinator/module_ibc.go (12)
69-74: Channel ordering validation is correctThe code properly checks that the channel ordering is
ORDERED, ensuring consistent packet delivery.
77-83: Port ID verification ensures module integrityBy confirming that the
portIDmatches the module's bound port, the code prevents misrouting of IBC packets.
85-92: Counterparty version validation promotes compatibilityEnsuring the
counterpartyVersionmatchescommontypes.Versionhelps maintain protocol consistency between chains.
94-101: Counterparty port ID check enhances securityValidating that the
counterparty.PortIdiscommontypes.SubscriberPortIDensures communication with the correct module on the counterparty chain.
103-108: Channel capability claim is necessary for authenticationClaiming the channel capability using
im.keeper.ClaimCapabilitysecures the module's access to the channel.
110-114: Subscriber chain verification adds an extra security layerCalling
VerifySubscriberChainconfirms the connection is established with the intended subscriber chain.
116-123: Handshake metadata is correctly constructed and returnedThe metadata includes the coordinator fee pool address and version, facilitating proper initialization of the IBC connection.
193-197: Safe unmarshalling of packet dataUsing
UnmarshalJSONfor packet data ensures robustness against improperly formed packets from the subscriber chain.
206-222: Comprehensive handling of packet types inOnRecvPacketThe switch statement processes known packet types and includes a default case for unknown types, returning an appropriate error. This aligns with best practices for packet handling.
225-232: Acknowledgement is consistently initializedBy initializing
ackin all cases, the code avoids potential nil pointer dereferences and ensures reliable acknowledgment handling.
255-261: Proper error handling when unmarshalling acknowledgmentsReturning a wrapped error if unmarshalling fails helps diagnose issues with acknowledgment data and maintains robustness.
275-290: Detailed event emission based on acknowledgment responseDifferentiating between success and error responses enhances observability and aids in monitoring the IBC interactions.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/appchain/subscriber/keeper/validators.go (1)
31-99: LGTM with a minor optimization suggestionThe
ApplyValidatorChangesfunction correctly processes validator updates, handling various scenarios such as updating, deleting, and adding validators. The error handling and logging are appropriate.Consider pre-allocating the
retslice with the expected capacity to avoid potential reallocation:-ret := make([]abci.ValidatorUpdate, 0, len(changes)) +ret := make([]abci.ValidatorUpdate, 0, len(changes))This minor optimization can slightly improve performance, especially for large validator sets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- x/appchain/coordinator/module_ibc.go (1 hunks)
- x/appchain/subscriber/keeper/validators.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/appchain/coordinator/module_ibc.go
🧰 Additional context used
🔇 Additional comments (7)
x/appchain/subscriber/keeper/validators.go (7)
11-16: LGTM: Correct implementation of SetValsetUpdateIDForHeightThe function correctly sets the valset update ID for a given height in the key-value store. It uses appropriate serialization and key generation methods.
19-29: LGTM: Correct implementation of GetValsetUpdateIDForHeightThe function correctly retrieves the valset update ID for a given height from the key-value store. It handles the case where the key doesn't exist and properly deserializes the value when found.
101-109: LGTM: Correct implementation of SetSubscriberChainValidatorThe function properly serializes and stores the validator in the key-value store using the appropriate key generation method.
111-124: LGTM: Correct implementation of GetSubscriberChainValidatorThe function correctly retrieves and deserializes the validator data from the key-value store. It properly handles the case where the validator is not found.
126-135: LGTM: Improved implementation of DeleteSubscriberChainValidatorThe function now checks for the existence of the validator before attempting to delete it, addressing the concern raised in a previous review. It also logs a message if the validator is not found, which is helpful for debugging and auditing purposes.
137-152: LGTM: Correct and efficient implementation of GetAllSubscriberChainValidatorsThe function efficiently retrieves all validators from the key-value store using a prefix iterator. It correctly deserializes each validator and properly manages the iterator's lifecycle.
1-152: Overall: Well-implemented validator management functionsThis file provides a comprehensive set of functions for managing subscriber chain validators. The implementations are robust, efficient, and follow good practices for key-value store operations and error handling. Notable points:
- Proper serialization and deserialization of data.
- Consistent use of helper functions for key generation.
- Appropriate error handling and logging.
- Efficient use of iterators for bulk operations.
The minor optimization suggestion for
ApplyValidatorChangesand the improvement inDeleteSubscriberChainValidatorfurther enhance the quality of the code.
MaxMustermann2
requested review from
rlan35,
bwhour,
cloud8little,
trestinlsd,
mikebraver,
TimmyExogenous and
leonz789
| // is valid before calling this function. The function forwards the slashing request to the | ||
| // operator module, which will trigger a slashing hook and thus a validator set update containing | ||
| // the slashing acknowledgment. | ||
| func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data commontypes.SlashPacketData) { |
There was a problem hiding this comment.
scalability optimization: should we add SlashStrategy interface to support different slash strategy? like
`Custom punishment policies are supported
type SlashStrategy interface {
// Calculate the penalty amount
CalculateSlash(ctx sdk. Context, validator sdk. ValAddress) sdk. Dec
// Gets the penalty type
GetInfractionType() InfractionType
// Get the minimum penalty threshold
GetMinSlashAmount() sdk. Dec
}
type InfractionType string
const (
InfractionDowntime InfractionType = "downtime"
InfractionDoubleSign InfractionType = "double_sign"
InfractionMisbehavior InfractionType = "misbehavior"
)`
in this keeper we can register such SlashStrategyManager
type SlashStrategyManager struct {
strategies map[InfractionType]SlashStrategy
}
so in HandleSlashPacket we can obtain the corresponding penalty strategy and then do strategy.CalculateSlash
There was a problem hiding this comment.
I think this will need to be a wider change across x/avs and x/operator for slashing.
CC @TimmyExogenous
There was a problem hiding this comment.
For now, it seems unnecessary. For app chain types of AVS and Dogfood, Cosmos-SDK already provides a defined slash type and execution method. For future AVS types, we may need a feature allowing AVS to submit their supported slash types along with corresponding slash proportions or slash penalty thresholds. Execution would then follow parameters configured by the AVS. In terms of implementation, using the interface abstraction described here may not be necessary, as the current slash execution relies on external input of slash type and proportion, which should basically meet the slash requirements of all AVS. Naturally, if future AVS have slash strategy requirements that significantly differ from the current implementation, the aforementioned extensibility modifications might be needed, but such changes should only be implemented if these cases arise.
For the current slash implementation, the primary focus might be on whether jail should be extended to cover slashes from all AVS to achieve a unified frozen feature, i.e., integrating jail as part of the base-level slash implementation available to all AVS. Additionally, it is crucial to consider how different AVS can define and validate malicious behavior conveniently to trigger slash execution. This is the key area where the future slash implementation needs thoughtful design and abstraction.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
x/appchain/subscriber/types/validator.go (1)
34-46: Consider adding error documentation.The error conditions and types should be documented in the function comment to help API consumers handle errors appropriately.
// ConsPubKey returns the validator PubKey as a cryptotypes.PubKey. +// Returns sdkerrors.ErrInvalidType if the cached value is not of type cryptotypes.PubKey. func (scv SubscriberChainValidator) ConsPubKey() (cryptotypes.PubKey, error) {x/dogfood/keeper/msg_server.go (1)
118-124: Consider enhancing error message clarity.While the error handling is correct, the error message could be more descriptive about why the chain ID validation failed.
- return nil, errors.Wrapf( - types.ErrNotAVSByChainID, "chainID:%s avsAddr:%s", c.ChainID(), avsAddr, - ) + return nil, errors.Wrapf( + types.ErrNotAVSByChainID, + "chain ID %q is not registered as an AVS (address: %s)", + c.ChainID(), + avsAddr, + )x/avs/keeper/avs.go (1)
Line range hint
89-106: Consider implementing reverse mapping for task address lookups.The TODO comment indicates a performance concern with iterating over all AVS entries to find one by task address. This could become a bottleneck as the number of AVS entries grows.
Consider implementing a reverse mapping from task address to AVS info to achieve O(1) lookup time instead of the current O(n) implementation. This would require:
- Adding a new prefix store for task address to AVS mapping
- Updating the mapping during AVS registration and updates
- Using direct lookup instead of iteration
Would you like me to create a GitHub issue to track this performance improvement?
utils/utils.go (1)
248-254: Consider pre-allocating the output slice for better performanceWhile the implementation is correct, pre-allocating the output slice could improve performance by reducing reallocations.
func AppendMany(slices ...[]byte) (out []byte) { + // Calculate total length + totalLen := 0 + for _, slice := range slices { + totalLen += len(slice) + } + // Pre-allocate the slice + out = make([]byte, 0, totalLen) for _, slice := range slices { out = append(out, slice...) } return out }x/avs/keeper/keeper.go (3)
Line range hint
236-238: Add bounds checking for task periodsThe task periods (challenge, response, statistical) should include validation to ensure:
- Values are positive and non-zero
- Values don't exceed reasonable maximum bounds
- The sum of periods doesn't exceed the AVS unbonding period
task := &types.TaskInfo{ + // Add validation before creating TaskInfo + if params.TaskChallengePeriod == 0 || params.TaskResponsePeriod == 0 || params.TaskStatisticalPeriod == 0 { + return errorsmod.Wrap(types.ErrInvalidTaskPeriod, "periods must be greater than zero") + } + if params.TaskChallengePeriod + params.TaskResponsePeriod + params.TaskStatisticalPeriod > avsInfo.AvsUnbondingPeriod { + return errorsmod.Wrap(types.ErrInvalidTaskPeriod, "sum of periods exceeds AVS unbonding period") + } Name: params.TaskName,
Line range hint
266-271: Enhance BLS signature verification error handlingThe current error handling could be more descriptive to aid in debugging signature verification failures.
valid, err := blst.VerifySignature(sig, [32]byte(msgHash), pubKey) if err != nil || !valid { - return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf("the operator is :%s", params.Operator)) + if err != nil { + return errorsmod.Wrapf(types.ErrSigNotMatchPubKey, "signature verification failed for operator %s: %v", params.Operator, err) + } + return errorsmod.Wrapf(types.ErrSigNotMatchPubKey, "invalid signature for operator %s", params.Operator) }
Line range hint
449-463: Simplify challenge period validation logicThe current challenge period validation logic is complex and could be refactored for better readability and maintainability.
- if epoch.CurrentEpoch <= int64(taskInfo.StartingEpoch)+int64(taskInfo.TaskResponsePeriod)+int64(taskInfo.TaskStatisticalPeriod) { - return errorsmod.Wrap( - types.ErrSubmitTooSoonError, - fmt.Sprintf("SetTaskResultInfo:the challenge period has not started , CurrentEpoch:%d", epoch.CurrentEpoch), - ) - } - if epoch.CurrentEpoch > int64(taskInfo.StartingEpoch)+int64(taskInfo.TaskResponsePeriod)+int64(taskInfo.TaskStatisticalPeriod)+int64(taskInfo.TaskChallengePeriod) { - return errorsmod.Wrap( - types.ErrSubmitTooLateError, - fmt.Sprintf("SetTaskResultInfo:submit too late, CurrentEpoch:%d", epoch.CurrentEpoch), - ) - } + challengeStartEpoch := int64(taskInfo.StartingEpoch) + int64(taskInfo.TaskResponsePeriod) + int64(taskInfo.TaskStatisticalPeriod) + challengeEndEpoch := challengeStartEpoch + int64(taskInfo.TaskChallengePeriod) + + if epoch.CurrentEpoch <= challengeStartEpoch { + return errorsmod.Wrapf(types.ErrSubmitTooSoonError, + "challenge period has not started (current: %d, start: %d)", + epoch.CurrentEpoch, challengeStartEpoch) + } + + if epoch.CurrentEpoch > challengeEndEpoch { + return errorsmod.Wrapf(types.ErrSubmitTooLateError, + "challenge period has ended (current: %d, end: %d)", + epoch.CurrentEpoch, challengeEndEpoch) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
precompiles/avs/avs_test.go(1 hunks)utils/utils.go(2 hunks)x/appchain/subscriber/types/validator.go(1 hunks)x/avs/keeper/avs.go(2 hunks)x/avs/keeper/keeper.go(2 hunks)x/dogfood/keeper/msg_server.go(1 hunks)x/operator/keeper/grpc_query.go(6 hunks)x/operator/keeper/slash.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- precompiles/avs/avs_test.go
🧰 Additional context used
📓 Learnings (2)
utils/utils.go (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#176
File: utils/utils.go:259-265
Timestamp: 2024-11-12T10:03:10.791Z
Learning: The `IsRevisionFormat` function enforces that chain IDs are in the format `{chainID}-{revision}`, ensuring that base chain IDs do not contain additional hyphens.
x/appchain/subscriber/types/validator.go (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#176
File: x/appchain/subscriber/types/validator.go:13-18
Timestamp: 2024-11-12T10:03:05.536Z
Learning: In Go, when using `codectypes.NewAnyWithValue(pubKey)` from the Cosmos SDK, an explicit nil check for `pubKey` is unnecessary because the function already handles nil values by returning an error.
🔇 Additional comments (9)
x/appchain/subscriber/types/validator.go (2)
11-25: LGTM! Constructor implementation is correct and follows best practices.
The constructor properly handles error cases and initializes all fields. The implementation correctly uses NewAnyWithValue which handles nil checks internally.
27-32: LGTM! UnpackInterfaces implementation is concise and correct.
The implementation follows the UnpackInterfacesMessage interface requirements and is properly documented.
x/avs/keeper/avs.go (1)
135-135: LGTM! Consistent usage of utility function.
The change correctly uses the centralized utility function for chain ID processing.
x/operator/keeper/grpc_query.go (1)
59-59: LGTM! Consistent usage of chain ID utility function
The changes consistently replace the chain ID handling with utils.ChainIDWithoutRevision across all query methods. This aligns with the codebase-wide standardization of chain ID processing.
Also applies to: 86-86, 110-110, 147-147, 206-206
utils/utils.go (2)
216-246: LGTM! Well-implemented validator update accumulation
The implementation correctly handles:
- Uniqueness using map with pubkey as key
- Deterministic sorting by power and pubkey
- Clear comments explaining the necessity of sorting
256-264: LGTM! Chain ID handling is correct
The implementation correctly handles chain ID revision format, as confirmed by the learnings about IsRevisionFormat enforcing the {chainID}-{revision} format.
x/operator/keeper/slash.go (1)
188-188: LGTM! Consistent chain ID handling
The change to use utils.ChainIDWithoutRevision aligns with the standardized approach to chain ID processing across the codebase.
x/avs/keeper/keeper.go (2)
16-16: Verify the ChainIDWithoutRevision function migration
The ChainIDWithoutRevision function has been moved from the types package to the utils package. Let's verify that all usages have been updated consistently.
✅ Verification successful
All references to ChainIDWithoutRevision have been updated to use utils package
The search results confirm that all occurrences of ChainIDWithoutRevision in the codebase are using the utils package, and there are no remaining references to types.ChainIDWithoutRevision. The function migration has been completed consistently across all files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to types.ChainIDWithoutRevision
rg "types\.ChainIDWithoutRevision"
# Search for all current uses of utils.ChainIDWithoutRevision
rg "utils\.ChainIDWithoutRevision"
Length of output: 4053
95-98: 🛠️ Refactor suggestion
Improve epoch handling logic
The current implementation has two issues:
- The TODO comment suggests that the current epoch handling needs improvement
- The condition for setting
startingEpochtocurrentEpochwhen chain IDs match lacks proper documentation explaining the rationale
Consider:
- Documenting why the starting epoch is set differently for matching chain IDs
- Adding validation to ensure this special case doesn't cause issues with AVS synchronization
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs. |
The key functionalities added in this PR are:
Next parts will contain
Summary by CodeRabbit
Summary by CodeRabbit
New Features
SubscriberChainValidator, enhancing validator management.Improvements
Bug Fixes
Documentation
Refactor