feat: revamp permissions component #14
Conversation
|
@im-adithya we had a meeting with @stackingsaunter and @reneaaron to review this, and the implementation is not the same as I thought we agreed upon. If an app passes anything in, then that should not be able to be edited. And the scope group of "Send and Receive" makes no sense to me as a default if it discards ALL the scopes except for those two scopes. Why do we do that? why do we want to restrict apps to only these two scopes by default? |
| }; | ||
|
|
||
| export const scopeGroupDescriptions: Record<ScopeGroupType, string> = { | ||
| [SCOPE_GROUP_FULL_ACCESS]: "Complete wallet control", |
There was a problem hiding this comment.
Review scope group descriptions
| NIP_47_GET_INFO_METHOD, | ||
| NIP_47_LOOKUP_INVOICE_METHOD, | ||
| NIP_47_LIST_TRANSACTIONS_METHOD, | ||
| NIP_47_NOTIFICATIONS_PERMISSION, |
There was a problem hiding this comment.
Are these good for Read Only Scopes?
There was a problem hiding this comment.
note: the variables you used are not even scopes (as you see _METHOD etc)
| )} | ||
| {capabilities.scopes.includes(NIP_47_PAY_INVOICE_METHOD) && | ||
| permissions.scopes.has(NIP_47_PAY_INVOICE_METHOD) && | ||
| (!isBudgetAmountEditable ? ( |
There was a problem hiding this comment.
If budget amount is set and budget renewal isn't (in the params) then it assumes renewal as "Monthly" and the user can't change it. Avoiding option to edit budget renewal in these cases as that adds unnecessary complexity.
There was a problem hiding this comment.
I am wondering if the default should be never in this case
There was a problem hiding this comment.
Yeah, I believe default should be never. Budgets from a deeplink-created app are different than the default user flow, and monthly in this case does not make sense
| ); | ||
| const [isExpiryEditable, setExpiryEditable] = React.useState( | ||
| isNewConnection ? !initialPermissions.expiresAt : canEditPermissions | ||
| ); |
There was a problem hiding this comment.
Current flow (given nothing is passed in parameters)
permissions are set in NewApp as
scopes: []
maxAmount:NaN (as AppPermissions type expects a number)
expiresAt: undefined (thankfully it expects Date | undefined)
And hence we use these checks to identify if the params had any permission data
There was a problem hiding this comment.
Using NaN seems a bit strange, I'll take a look and see if there's another way
| const handlePermissionsChange = React.useCallback( | ||
| (changedPermissions: Partial<AppPermissions>) => { | ||
| const updatedPermissions = { ...permissions, ...changedPermissions }; | ||
| setPermissions(updatedPermissions); |
There was a problem hiding this comment.
Setting local permissions separately as depending on initialPermissions might cause issues due to param data being changed. (Param data is only set from permissions and not initialPermissions so it doesn't change when it is updated, and the isEditable states are not disturbed post that in NewApp screen)
| try { | ||
| const createAppRequest: CreateAppRequest = { | ||
| name: appName, | ||
| pubkey, | ||
| budgetRenewal: permissions.budgetRenewal, | ||
| maxAmount: permissions.maxAmount, | ||
| maxAmount: permissions.maxAmount || 0, |
There was a problem hiding this comment.
^if user doesn't change and it stays in NaN
This whole NaN thing feels hacky, but changing maxAmount to number | undefined would need a lot of changes
| }, [capabilities.scopes]); | ||
|
|
||
| const [scopeGroup, setScopeGroup] = React.useState<ScopeGroup>(() => { | ||
| if (!scopes.size || isSetEqual(scopes, fullAccessScopes)) { |
There was a problem hiding this comment.
We don't need to do this, if an app passes in any request methods it can just be the custom option
| return SCOPE_GROUP_CUSTOM; | ||
| }); | ||
|
|
||
| // we need scopes to be empty till this point for isScopesEditable |
There was a problem hiding this comment.
this comment sounds like a hack
same with // stop setting scopes on re-renders
| export const NIP_47_MULTI_PAY_KEYSEND_METHOD = "multi_pay_keysend"; | ||
| export const NIP_47_MULTI_PAY_INVOICE_METHOD = "multi_pay_invoice"; | ||
|
|
||
| export const SCOPE_GROUP_FULL_ACCESS = "full_access"; |
There was a problem hiding this comment.
We should try to only put types here which are used in multiple files.
Also, there is a mix of these constant variables, and string unions. We should be consistent here
| ) { | ||
| scopes.add("pay_invoice"); | ||
| scopes.add(NIP_47_PAY_INVOICE_METHOD); |
There was a problem hiding this comment.
Scopes are not the same as methods.
|
@im-adithya there are a lot of issues here still and some things that are fundamentally wrong, and this will take too long to fix if I keep adding review comments like this. We also need to ensure this works with the new isolated app type, so I have started a new branch off of this one, started cleaning things up, and have also merged the dynamic budgets PR into it. I will be continuing there: #273 Please read through my commits to get an idea of what I have changed and let me know if you have any questions. |
Screenshots
No Params
max_amount=172166expires_at=1721673000request_methods=make_invoice pay_invoice&max_amount=172166request_methods=make_invoice pay_invoice&expires_at=1721673000request_methods=make_invoice pay_invoice&expires_at=1721673000request_methods=make_invoice pay_invoice&expires_at=1721673000&max_amount=212121