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

fix: collateral password issue [LW-11953] #1584

Merged
merged 2 commits into from
Dec 10, 2024
Merged

fix: collateral password issue [LW-11953] #1584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6650980
fix: collateral password issue
greatertomi Dec 9, 2024
f3a977c
fix: resolve build issue on main
greatertomi Dec 9, 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
3 changes: 1 addition & 2 deletions .../views/browser-view/features/settings/components/Collateral/send/CollateralFooterSend.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,14 @@ export const CollateralFooterSend = ({
const submitTx = async () => withSignTxConfirmation(submitCollateralTx, secretsUtil.password.value);

const handleClick = async () => {
onClaim();

if (!hasEnoughAda) {
return onClose();
}
try {
if (popupView && !isInMemory)
return await backgroundServices?.handleOpenBrowser({ section: BrowserViewSections.COLLATERAL_SETTINGS });
await submitTx();
onClaim();
greatertomi marked this conversation as resolved.
Show resolved Hide resolved
toast.notify({ text: t('browserView.settings.wallet.collateral.toast.add') });
if (isInMemory) onClose();
} catch {
Expand Down
5 changes: 5 additions & 0 deletions ...re/src/shared-wallets/add-shared-wallet/generate-key-flow/EnterPassword/EnterPassword.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ export const EnterPassword: VFC<EnterPasswordProps> = ({
const icon = mapOfWalletTypeIconProperties[kind];

const next = () => {
if (!password.value) {
console.error('Password is undefined');
Copy link
Contributor

@pczeglik-iohk pczeglik-iohk Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

console.error is commonly used in the lace codebase, so I think it's fine.

But in the same time I wonder if this is the way we should handle it. Especially as we have Sentry in place.

Maybe we do not need to pollute users console with application error, but report them to Sentry, so in this case throw new Error('EnterPassword: password is undefined') maybe a bit better.

It can be as it is, no need to change it right now, but it's worth exploring this area in upcoming weeks, so we can refactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful with throwing errors in react code, as it might unmount the whole app, as we don't have any error boundary as far as I know. The disappearing application would be a far more severe issue than the benefit of catching the problem with sentry.

Copy link
Contributor

Choose a reason for hiding this comment

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

there are methods like captureException and captureMessage in Sentry https://docs.sentry.io/platforms/javascript/guides/react/usage/
we may be interested to use. We could wrap it into some abstraction layer so we are provider agnostic etc

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely worth exploring. I just want to mention that I would prefer to have a seamless implementation instead of some function that I need to explicitly call in order to register the exception in sentry. I would like to stick to native mechanism - just throw an error that would be catched at some higher level and reported, instead of remembering that I need to pass it to some function.

return;
}

onGenerateKeys(password.value);
clearSecrets();
};
Expand Down
9 changes: 4 additions & 5 deletions packages/core/src/ui/components/WalletSetup/WalletSetupPasswordStep.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,9 @@ export const WalletSetupPasswordStep = ({
const passwordConfirmationErrorMessage =
passHasBeenValidated && password !== passwordConfirmation ? translations.noMatchPassword : '';
const isNextEnabled = Boolean(
passHasBeenValidated &&
!passwordConfirmationErrorMessage &&
score >= minimumPassLevelRequired &&
password.value.length > 0
greatertomi marked this conversation as resolved.
Show resolved Hide resolved
passHasBeenValidated && !passwordConfirmationErrorMessage && score >= minimumPassLevelRequired && password?.value
? password.value.length > 0
: false
);

const complexityBarList: BarStates = useMemo(() => getComplexityBarStateList(score), [score]);
Expand All @@ -79,7 +78,7 @@ export const WalletSetupPasswordStep = ({
description={translations.description}
onBack={onBack}
onNext={() => {
onNext({ password: password.value });
password.value ? onNext({ password: password.value }) : console.error('Password is undefined');
}}
isNextEnabled={isNextEnabled}
currentTimelineStep={WalletTimelineSteps.WALLET_SETUP}
Expand Down
Loading