Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new feature gate to control SAE behavior by disabling optimistic confirmations when the flag is enabled.
Changes:
- Introduces
FeatureGates.FORCE_SAEin shared feature flag types. - Adds default/disabled values for the new gate in common feature flag constants.
- Updates
useIsOptimisticConfirmationEnabledto short-circuit (returnfalse) whenFORCE_SAEis enabled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/ui/src/hooks/useIsOptimisticConfirmationEnabled.ts | Reads the new feature flag and disables optimistic confirmations when enabled. |
| packages/types/src/feature-flags.ts | Adds the FORCE_SAE feature gate enum value. |
| packages/common/src/feature-flags.ts | Registers the new gate in DISABLED_FLAG_VALUES and DEFAULT_FLAGS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isForceSaeEnabled = featureFlags[FeatureGates.FORCE_SAE]; | ||
|
|
||
| return useCallback( | ||
| async (network?: NetworkWithCaipId) => { | ||
| if (isForceSaeEnabled) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
FeatureGates.FORCE_SAE is used here to force useIsOptimisticConfirmationEnabled to return false (i.e., disable optimistic confirmations). The flag name reads like an enablement gate, so the intent is ambiguous for future readers. Consider either renaming the gate/variable to reflect that it disables optimistic confirmations, or add an inline comment explaining the relationship between “SAE” and optimistic confirmations.
| const isForceSaeEnabled = featureFlags[FeatureGates.FORCE_SAE]; | ||
|
|
||
| return useCallback( | ||
| async (network?: NetworkWithCaipId) => { | ||
| if (isForceSaeEnabled) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
New behavior is introduced via FeatureGates.FORCE_SAE, but there’s no unit test coverage verifying that the hook returns false when this flag is enabled (and ideally that it short-circuits without hitting provider.getInfo()). Please add a Jest test for this hook similar to other hook tests in packages/ui/src/hooks.
Description
Changes
Testing
Screenshots:
Checklist for the author
Tick each of them when done or if not applicable.