feat: notifications integration sdk #9673
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is new author?A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package. Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
| @@ -1,12 +1,14 @@ | |||
| import { ACTIONS } from '../../reducers/notification'; | |||
There was a problem hiding this comment.
Since we are shipping notification it would be cool to refactor this to use the slice pattern with RTK we have some examples on the codebase like this one:
It would be cool to instead of the action and reducer we have a single file with the slice instead, in typescript, let me know your thoughts!
There was a problem hiding this comment.
@tommasini what is the benefit of this slice pattern?
| return type === NETWORK_TYPES.RPC ? rpcUrl : type; | ||
| }, | ||
| getCurrentChainId: () => networkController.state.providerConfig.chainId, | ||
| environment: process.env.METAMASK_ENVIRONMENT, |
There was a problem hiding this comment.
should we have a default case here? I am not sure how reliable this value is.
There was a problem hiding this comment.
Well, this is also implemented here and used everywhere within the app through the call of const providerConfig = useSelector(selectProviderConfig);. What do you advice?
There was a problem hiding this comment.
I was referring to process.env.METAMASK_ENVIRONMENT
There was a problem hiding this comment.
If its too much trouble than its okay.
| * @returns An object containing: | ||
| * - `createSession`: A function to initiate the session creation process. | ||
| */ | ||
| function useCreateSession(): { |
There was a problem hiding this comment.
I really love the hooks that you have created. I strongly encourage you to write some basic hook tests using renderHook. This goes for all of the hooks in the new hooks folder. Here is an example in the codebase.
There was a problem hiding this comment.
I did this already here. Is there any other approach you're suggesting?
There was a problem hiding this comment.
Sorry I did not see this before. This is great. I guess I was looking for a specific useCreateSession.test.ts file but this work. I think having individual test files is cleaner and easier to maintain as we scale but I do not feel strongly about it.
| export type AccountType = InternalAccount & { | ||
| balance: string; | ||
| keyring: KeyringType; | ||
| label: string; |
There was a problem hiding this comment.
I am pretty sure that the InternalAccount object has the label (defined as name) under the metadata object.
There was a problem hiding this comment.
That's a good point. On the extension notification's implementation they are using name and label here. So I don't know if on purpose or by mistake. Meanwhile, I'll put the question to the Notification's team.
There was a problem hiding this comment.
I can confirm that the account name (same as label) is already available on the InternalAccounts object. In my opinion we should rely on this object as much as possible for accounts related work. The balance will need to be added but we can get the label from metadata. The type is also available on this interface but it has support for non evm account type so if we want to limit it to only evm we can use KeyringType
Description
This PR implements all the REDUX flows (Actions, Reducers, Sagas, Controllers usage) to power Notifications UI.
Related issues
Fixes: N/A
Manual testing steps
Screenshots/Recordings
Before
N/A
After
N/A
Pre-merge author checklist
Pre-merge reviewer checklist