Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive escrow delegate management system with granular permissions, allowing escrow owners to grant specific operational permissions to trusted addresses without transferring full ownership.
Key Changes:
- Added a bitwise permission system with four permission types: createJobs, extendDuration, extendNodes, and redeemUnused
- Implemented delegate management UI in the Account page with CRUD operations for delegates
- Updated authentication flow to fetch and store user permissions during login
- Added permission checks to protected pages (ExtendJob, EditJob, Project, Monitor)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/permissions/delegates.ts |
Defines permission types and bitwise operations for delegate access control |
src/lib/contexts/deployment/context.ts |
Adds EscrowAccess type and permission-related context properties |
src/lib/contexts/deployment/deployment-provider.tsx |
Implements fetchEscrowAccess and hasEscrowPermission functions |
src/components/account/delegates/EscrowDelegates.tsx |
New component for managing delegate addresses and permissions with full CRUD operations |
src/pages/Account.tsx |
Adds "Delegates" tab to account management interface |
src/pages/deeploys/job/ExtendJob.tsx |
Enforces extendDuration permission with early return pattern |
src/pages/deeploys/job/EditJob.tsx |
Enforces extendNodes permission with early return pattern |
src/pages/deeploys/Project.tsx |
Enforces createJobs permission with early return pattern |
src/pages/deeploys/Monitor.tsx |
Enforces redeemUnused permission with early return pattern |
src/pages/Login.tsx |
Updates authentication to fetch escrow access and permissions, allowing delegate login |
src/blockchain/PoAIManager.ts |
Adds ABI definitions for getAddressRegistration and delegate-related functions |
src/blockchain/CspEscrow.ts |
Adds ABI definitions for getDelegatePermissions, getDelegatedAddresses, setDelegatePermissions, and removeDelegate |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div | ||
| key={delegate.address} | ||
| className="row w-full flex-wrap items-center justify-between gap-3 rounded-xl border border-slate-100 bg-white px-4 py-3 hover:border-slate-200" | ||
| onClick={() => openEditModal(delegate)} | ||
| > | ||
| <div className="col gap-1"> | ||
| <CopyableValue value={delegate.address}> | ||
| <div className="font-roboto-mono text-sm font-medium"> | ||
| {getShortAddressOrHash(delegate.address, 6, true)} | ||
| </div> | ||
| </CopyableValue> | ||
| </div> | ||
|
|
||
| <div className="row flex-wrap gap-1.5"> | ||
| {activePermissions.length ? ( | ||
| activePermissions.map((permission) => ( | ||
| <SmallTag key={permission.key}>{permission.label}</SmallTag> | ||
| )) | ||
| ) : ( | ||
| <SmallTag>No permissions</SmallTag> | ||
| )} | ||
| </div> | ||
|
|
||
| <div className="row gap-2"> | ||
| <Button | ||
| size="sm" | ||
| variant="flat" | ||
| startContent={<RiPencilLine />} | ||
| onPress={() => { | ||
| openEditModal(delegate); | ||
| }} | ||
| > | ||
| Edit | ||
| </Button> | ||
| <Button | ||
| size="sm" | ||
| color="danger" | ||
| variant="flat" | ||
| startContent={<RiDeleteBinLine />} | ||
| onPress={() => { | ||
| removeDelegate(delegate.address); | ||
| }} | ||
| isDisabled={!canManageDelegates || removingAddress === delegate.address} | ||
| isLoading={removingAddress === delegate.address} | ||
| > | ||
| Remove | ||
| </Button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The entire delegate row has an onClick handler that opens the edit modal, which may create confusing UX since there are also explicit "Edit" and "Remove" buttons within the row. Clicking the Remove button will trigger both the row's onClick and the button's onPress, potentially opening the edit modal while removing.
Consider removing the onClick from the row and relying only on the explicit button actions, or add event.stopPropagation() to the button handlers.
| if (!trimmedAddress || !trimmedAddress.startsWith('0x') || trimmedAddress.length !== 42) { | ||
| toast.error('Enter a valid delegate address.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The address validation only checks for basic format (starts with '0x' and length is 42). Consider using viem's isAddress() function for more robust validation, including checksum validation. This would prevent issues with invalid addresses being submitted to the smart contract.
Example:
import { isAddress } from 'viem';
if (!isAddress(trimmedAddress)) {
toast.error('Enter a valid delegate address.');
return;
}| placeholder="0x..." | ||
| value={formAddress} | ||
| onValueChange={(value) => setFormAddress(value)} | ||
| isDisabled={!canManageDelegates || isSaving} |
There was a problem hiding this comment.
When editing an existing delegate, the address field should be disabled to prevent confusion. The smart contract's setDelegatePermissions function updates permissions for a given address, so the address cannot be changed. Users should only be able to modify permissions when editing.
Add isDisabled={!canManageDelegates || isSaving || !!editingAddress} to make it clear the address is immutable when editing.
| isDisabled={!canManageDelegates || isSaving} | |
| isDisabled={!canManageDelegates || isSaving || !!editingAddress} |
| if (!trimmedAddress || !trimmedAddress.startsWith('0x') || trimmedAddress.length !== 42) { | ||
| toast.error('Enter a valid delegate address.'); | ||
| return; | ||
| } | ||
|
|
||
| if (permissionsValue === 0n) { | ||
| toast.error('Select at least one permission.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Consider adding validation to prevent adding the owner's own address as a delegate, as the owner already has full permissions. Also, add validation to prevent the zero address (0x0000000000000000000000000000000000000000).
Example:
if (trimmedAddress.toLowerCase() === ownerAddress?.toLowerCase()) {
toast.error('Cannot add owner address as delegate.');
return;
}
if (isZeroAddress(trimmedAddress)) {
toast.error('Cannot add zero address as delegate.');
return;
}| if (!hasEscrowPermission('extendDuration')) { | ||
| return ( | ||
| <div className="center-all flex-1"> | ||
| <DetailedAlert | ||
| variant="red" | ||
| icon={<RiAlertLine />} | ||
| title="Permission required" | ||
| description={<div>You do not have permission to extend job duration.</div>} | ||
| isCompact | ||
| /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The permission check may show a false negative if the component renders before fetchEscrowAccess completes during login. When currentUserPermissions is undefined, hasEscrowPermission returns false, potentially showing "Permission required" briefly even to authorized users.
Consider showing a loading state when currentUserPermissions === undefined to distinguish between "loading permissions" and "lacks permission":
if (currentUserPermissions === undefined) {
return <ExtendJobPageLoading />;
}
if (!hasEscrowPermission('extendDuration')) {
return (
<div className="center-all flex-1">
<DetailedAlert ... />
</div>
);
}| if (!hasEscrowPermission('extendNodes')) { | ||
| return ( | ||
| <div className="center-all flex-1"> | ||
| <DetailedAlert | ||
| variant="red" | ||
| icon={<RiAlertLine />} | ||
| title="Permission required" | ||
| description={<div>You do not have permission to extend job nodes.</div>} | ||
| isCompact | ||
| /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The permission check may show a false negative if the component renders before fetchEscrowAccess completes during login. When currentUserPermissions is undefined, hasEscrowPermission returns false, potentially showing "Permission required" briefly even to authorized users.
Consider showing a loading state when currentUserPermissions === undefined to distinguish between "loading permissions" and "lacks permission":
if (currentUserPermissions === undefined) {
return <EditJobPageLoading />;
}
if (!hasEscrowPermission('extendNodes')) {
return (
<div className="center-all flex-1">
<DetailedAlert ... />
</div>
);
}| if (!hasEscrowPermission('createJobs')) { | ||
| return ( | ||
| <div className="center-all flex-1"> | ||
| <DetailedAlert | ||
| variant="red" | ||
| icon={<RiAlertLine />} | ||
| title="Permission required" | ||
| description={<div>You do not have permission to create new jobs.</div>} | ||
| isCompact | ||
| /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The permission check may show a false negative if the component renders before fetchEscrowAccess completes during login. When currentUserPermissions is undefined, hasEscrowPermission returns false, potentially showing "Permission required" briefly even to authorized users.
Consider showing a loading state when currentUserPermissions === undefined to distinguish between "loading permissions" and "lacks permission":
if (currentUserPermissions === undefined) {
return <ProjectPageLoading />;
}
if (!hasEscrowPermission('createJobs')) {
return (
<div className="center-all flex-1">
<DetailedAlert ... />
</div>
);
}| import { | ||
| ALL_DELEGATE_PERMISSIONS_MASK, | ||
| DelegatePermissionKey, | ||
| getPermissionValue, |
There was a problem hiding this comment.
Unused import getPermissionValue.
| getPermissionValue, |
No description provided.