fix: collateral password issue [LW-11953] #1584
Conversation
Allure Report
processReports: ✅ test report for f3a977c0
|
pczeglik-iohk
changed the title
...src/views/browser-view/features/settings/components/Collateral/send/CollateralFooterSend.tsx
Show resolved
Hide resolved
.../core/src/shared-wallets/add-shared-wallet/generate-key-flow/EnterPassword/EnterPassword.tsx
Outdated
Show resolved
Hide resolved
packages/core/src/ui/components/WalletSetup/WalletSetupPasswordStep.tsx
Outdated
Show resolved
Hide resolved
| @@ -44,6 +44,11 @@ export const EnterPassword: VFC<EnterPasswordProps> = ({ | |||
| const icon = mapOfWalletTypeIconProperties[kind]; | |||
|
|
|||
| const next = () => { | |||
| if (!password.value) { | |||
| console.error('Password is undefined'); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you @greatertomi Code looks good.
There is only one decision to be made for the future we can ignore right now.
greatertomi
force-pushed
the
fix/lw-11953-collateral-signing
branch
from
2dce5bf to
f3a977c
Compare
|
Checklist
Proposed solution
Adjusted the flow to submit transaction before clearing password