Wallet Detail - Action Summary & Action Logs for stewards #221 - #297
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @EmekaManuel - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/app/src/components/ActivityLog.tsx:67` </location>
<code_context>
+
+ const truncatedName = name.length > 15 ? `${name.substring(0, 5)}...${name.substring(name.length - 5)}` : name;
+
+ const handleIpfsPress = async () => {
+ if (ipfsHash) {
+ const cleanHash = ipfsHash.replace(/^(ipfs:\/\/|https?:\/\/[^/]+\/ipfs\/)/, '');
+ const gateways = [
+ `https://ipfs.io/ipfs/${cleanHash}`,
+ `https://gateway.pinata.cloud/ipfs/${cleanHash}`,
+ `https://cloudflare-ipfs.com/ipfs/${cleanHash}`,
+ ];
+
+ try {
+ await openExternalLink(gateways[0]);
+ } catch (error) {
+ try {
+ await openExternalLink(gateways[1]);
+ } catch (secondError) {
+ console.warn('IPFS gateways failed');
+ }
+ }
</code_context>
<issue_to_address>
IPFS gateway fallback logic may not work as intended due to openExternalLink always resolving.
Since openExternalLink only verifies if a URL can be opened, not if the resource exists, the fallback gateways may never be used even if the first is down. Consider checking resource availability with a fetch or HEAD request before attempting to open the link, or document that this is a best-effort fallback.
</issue_to_address>
### Comment 2
<location> `packages/app/src/components/ActivityLog.tsx:88` </location>
<code_context>
+ }
+ };
+
+ const hasPaymentData = Boolean(transactionHash || hash || owner);
+
return (
</code_context>
<issue_to_address>
hasPaymentData logic may enable Payment Transaction button even if only owner is present.
Currently, the button is enabled when only owner is present, which may not correspond to a transaction. Should the button be disabled unless a transaction hash exists?
</issue_to_address>
### Comment 3
<location> `packages/app/src/pages/ActivityLogPage.tsx:28` </location>
<code_context>
+
+ const { data: ensName } = useEnsName({ address, chainId: 1 });
+ const fullName = useFetchFullName(address);
+ const [firstName, lastName] = fullName?.trim().split(' ') ?? [undefined, undefined];
+ const userIdentifier = firstName ? `${firstName} ${lastName}` : ensName ?? address ?? '0x';
+ const isWhitelisted = useIsStewardVerified(address as `0x${string}`);
</code_context>
<issue_to_address>
Splitting fullName by space may not handle multi-part names correctly.
This approach may fail for names with more or fewer than two parts. Consider splitting on the first space or handling cases with missing or extra name parts.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
const fullName = useFetchFullName(address);
const [firstName, lastName] = fullName?.trim().split(' ') ?? [undefined, undefined];
const userIdentifier = firstName ? `${firstName} ${lastName}` : ensName ?? address ?? '0x';
=======
const fullName = useFetchFullName(address);
let firstName: string | undefined, lastName: string | undefined;
if (fullName && fullName.trim().length > 0) {
const trimmed = fullName.trim();
const firstSpaceIdx = trimmed.indexOf(' ');
if (firstSpaceIdx === -1) {
firstName = trimmed;
lastName = undefined;
} else {
firstName = trimmed.slice(0, firstSpaceIdx);
lastName = trimmed.slice(firstSpaceIdx + 1);
}
} else {
firstName = undefined;
lastName = undefined;
}
const userIdentifier = firstName ? (lastName ? `${firstName} ${lastName}` : firstName) : ensName ?? address ?? '0x';
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `packages/app/src/pages/ActivityLogPage.tsx:85` </location>
<code_context>
+ return selectedCollectiveActivities;
+ }
+
+ if (steward?.nfts && steward.nfts.length > 0) {
+ return steward.nfts
+ .filter((nft) => !selectedCollective || nft.collective === selectedCollective)
+ .map((nft, _index) => ({
+ id: nft.id,
+ name: nft.id,
+ creationDate: new Date().toLocaleDateString(),
+ nftId: nft.id,
+ hash: nft.hash,
+ owner: nft.owner,
+ collective: nft.collective,
+ ipfsHash: nft.hash,
+ }));
+ }
+
</code_context>
<issue_to_address>
Fallback to steward.nfts may result in duplicate or inconsistent activity data.
Please clarify which data source should take precedence and ensure the fallback logic does not cause data duplication or inconsistencies.
</issue_to_address>
### Comment 5
<location> `packages/app/src/hooks/useActivityLogData.ts:86` </location>
<code_context>
+ });
+}
+
+function formatNftId(nftId: string, collectiveId: string): string {
+ const prefix = collectiveId.substring(0, 3).toUpperCase();
+ const suffix = nftId.substring(nftId.length - 4);
+ const year = new Date().getFullYear();
+ return `#${prefix}-${year}-${suffix}`;
+}
+
</code_context>
<issue_to_address>
formatNftId uses current year, which may not match NFT creation year.
Consider passing the NFT's creation year to formatNftId to avoid displaying incorrect years for older NFTs.
</issue_to_address>
### Comment 6
<location> `packages/app/src/hooks/useActivityLogData.ts:109` </location>
<code_context>
+ return fallbackNames[pooltype] || 'Environmental Collective';
+}
+
+function formatAmount(amount: string): string {
+ try {
+ const amountBigInt = BigInt(amount);
+ const decimals = 18;
+ const divisor = BigInt(10 ** decimals);
+ const wholePart = amountBigInt / divisor;
+ const fractionalPart = amountBigInt % divisor;
+
+ if (fractionalPart === 0n) {
+ return wholePart.toString();
+ }
+
+ const fractionalStr = fractionalPart.toString().padStart(decimals, '0');
+ const trimmedFractional = fractionalStr.substring(0, 3).replace(/0+$/, '');
+
+ if (trimmedFractional === '') {
+ return wholePart.toString();
+ }
+
+ return `${wholePart}.${trimmedFractional}`;
+ } catch (error) {
+ return '0.758';
+ }
+}
</code_context>
<issue_to_address>
Hardcoded fallback value in formatAmount may be misleading.
Returning '0.758' on parse failure may mislead users. Use '0' or an explicit error indicator to avoid confusion.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
return '0.758';
=======
return '0';
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `packages/app/src/hooks/useActivityLogData.ts:20` </location>
<code_context>
+ timestamp: number;
+}
+
+export function useActivityLogData(stewardId: string): ActivityLogItem[] {
+ const activityData = useSubgraphStewardWithActivityData(stewardId);
+
</code_context>
<issue_to_address>
Consider extracting formatting and lookup helpers into separate utility modules and flattening nested loops with array methods to keep the hook focused on data mapping.
Here are two quick wins to collapse that hook down to just “data‐stuff” and move all the formatting/lookups out:
1) Extract your helpers into small utils modules
2) Flatten your nested loops with `flatMap` + `filter` + `map`
---
### 1) utils/formatDate.ts
```ts
// src/utils/formatDate.ts
export function formatDate(ts: number): string {
return new Date(ts * 1000).toLocaleDateString('en-US', {
year: 'numeric', month: 'long', day: 'numeric',
});
}
```
### 2) utils/formatNftId.ts
```ts
// src/utils/formatNftId.ts
export function formatNftId(nftId: string, collectiveId: string): string {
const prefix = collectiveId.slice(0,3).toUpperCase();
const suffix = nftId.slice(-4);
return `#${prefix}-${new Date().getFullYear()}-${suffix}`;
}
```
### 3) utils/formatAmount.ts
```ts
// src/utils/formatAmount.ts
export function formatAmount(amount: string): string {
try {
const value = BigInt(amount), dec = BigInt(10 ** 18);
const whole = value / dec;
const frac = (value % dec).toString().padStart(18,'0').slice(0,3).replace(/0+$/,'');
return frac ? `${whole}.${frac}` : whole.toString();
} catch {
return '0.758';
}
}
```
### 4) utils/names.ts
```ts
// src/utils/names.ts
const ACTIVITY: Record<string,string> = {
Climate: 'Silvi Proof of Planting',
DirectPayments: 'Environmental Action Proof',
UBI: 'UBI Claim',
};
const COLLECTIVE: Record<string,string> = {
Climate: 'Restoring the Kakamega Forest',
DirectPayments: 'Environmental Collective',
UBI: 'UBI Collective',
};
export function getActivityName(type: string) {
return ACTIVITY[type] || 'Environmental Action';
}
export function getCollectiveName(id: string, type: string) {
// if you eventually have a map of custom names, merge here:
return COLLECTIVE[type] || 'Environmental Collective';
}
```
---
### Updated hook (only data‐mapping)
```ts
import { useMemo } from 'react';
import { formatDate } from '../utils/formatDate';
import { formatNftId } from '../utils/formatNftId';
import { formatAmount } from '../utils/formatAmount';
import { getActivityName, getCollectiveName } from '../utils/names';
// ...
export function useActivityLogData(stewardId: string): ActivityLogItem[] {
const data = useSubgraphStewardWithActivityData(stewardId);
return useMemo(() => {
if (!data?.claims) return [];
return data.claims
.flatMap(claim =>
claim.events
.filter(evt =>
evt.nft &&
evt.contributors.some(c => c.id.toLowerCase() === stewardId.toLowerCase())
)
.map(evt => ({
id: evt.id,
name: getActivityName(claim.collective.pooltype),
creationDate: formatDate(evt.timestamp),
nftId: formatNftId(evt.nft.id, claim.collective.id),
nftHash: evt.nft.id,
ipfsHash: evt.nft.hash,
paymentAmount: `${formatAmount(evt.rewardPerContributor)} ${claim.settings.rewardToken}`,
transactionHash: claim.txHash,
collective: {
id: claim.collective.id,
name: getCollectiveName(claim.collective.id, claim.collective.pooltype),
},
timestamp: evt.timestamp,
}))
)
.sort((a, b) => b.timestamp - a.timestamp);
}, [data, stewardId]);
}
```
This way the hook stays focused on data‐flow, and all formatting/lookups live in tiny, reusable modules.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const handleIpfsPress = async () => { | ||
| if (ipfsHash) { | ||
| const cleanHash = ipfsHash.replace(/^(ipfs:\/\/|https?:\/\/[^/]+\/ipfs\/)/, ''); | ||
| const gateways = [ | ||
| `https://ipfs.io/ipfs/${cleanHash}`, | ||
| `https://gateway.pinata.cloud/ipfs/${cleanHash}`, | ||
| `https://cloudflare-ipfs.com/ipfs/${cleanHash}`, | ||
| ]; | ||
|
|
||
| try { |
There was a problem hiding this comment.
issue (bug_risk): IPFS gateway fallback logic may not work as intended due to openExternalLink always resolving.
Since openExternalLink only verifies if a URL can be opened, not if the resource exists, the fallback gateways may never be used even if the first is down. Consider checking resource availability with a fetch or HEAD request before attempting to open the link, or document that this is a best-effort fallback.
| } | ||
| }; | ||
|
|
||
| const hasPaymentData = Boolean(transactionHash || hash || owner); |
There was a problem hiding this comment.
question (bug_risk): hasPaymentData logic may enable Payment Transaction button even if only owner is present.
Currently, the button is enabled when only owner is present, which may not correspond to a transaction. Should the button be disabled unless a transaction hash exists?
| const fullName = useFetchFullName(address); | ||
| const [firstName, lastName] = fullName?.trim().split(' ') ?? [undefined, undefined]; | ||
| const userIdentifier = firstName ? `${firstName} ${lastName}` : ensName ?? address ?? '0x'; |
There was a problem hiding this comment.
suggestion: Splitting fullName by space may not handle multi-part names correctly.
This approach may fail for names with more or fewer than two parts. Consider splitting on the first space or handling cases with missing or extra name parts.
| const fullName = useFetchFullName(address); | |
| const [firstName, lastName] = fullName?.trim().split(' ') ?? [undefined, undefined]; | |
| const userIdentifier = firstName ? `${firstName} ${lastName}` : ensName ?? address ?? '0x'; | |
| const fullName = useFetchFullName(address); | |
| let firstName: string | undefined, lastName: string | undefined; | |
| if (fullName && fullName.trim().length > 0) { | |
| const trimmed = fullName.trim(); | |
| const firstSpaceIdx = trimmed.indexOf(' '); | |
| if (firstSpaceIdx === -1) { | |
| firstName = trimmed; | |
| lastName = undefined; | |
| } else { | |
| firstName = trimmed.slice(0, firstSpaceIdx); | |
| lastName = trimmed.slice(firstSpaceIdx + 1); | |
| } | |
| } else { | |
| firstName = undefined; | |
| lastName = undefined; | |
| } | |
| const userIdentifier = firstName ? (lastName ? `${firstName} ${lastName}` : firstName) : ensName ?? address ?? '0x'; |
| id: collective.collective, | ||
| name: stewardIpfsCollectives[index]?.name || `Collective ${index + 1}`, | ||
| count: collective.actions || steward.nfts?.length || 0, | ||
| })); |
There was a problem hiding this comment.
question (bug_risk): Fallback to steward.nfts may result in duplicate or inconsistent activity data.
Please clarify which data source should take precedence and ensure the fallback logic does not cause data duplication or inconsistencies.
| function formatNftId(nftId: string, collectiveId: string): string { | ||
| const prefix = collectiveId.substring(0, 3).toUpperCase(); | ||
| const suffix = nftId.substring(nftId.length - 4); | ||
| const year = new Date().getFullYear(); | ||
| return `#${prefix}-${year}-${suffix}`; |
There was a problem hiding this comment.
issue (bug_risk): formatNftId uses current year, which may not match NFT creation year.
Consider passing the NFT's creation year to formatNftId to avoid displaying incorrect years for older NFTs.
There was a problem hiding this comment.
True, nft's could have been created last year. please fix
There was a problem hiding this comment.
You're absolutely right—using the current year could misrepresent the actual creation year of the NFT. To fix this, you should update the formatNftId function to accept the NFT's creation timestamp (or year) as an argument, and use that value instead of new Date().getFullYear().
Here's how you could adjust the function:
function formatNftId(nftId: string, collectiveId: string, timestamp: number): string {
const prefix = collectiveId.substring(0, 3).toUpperCase();
const suffix = nftId.substring(nftId.length - 4);
const year = new Date(timestamp * 1000).getFullYear();
return `#${prefix}-${year}-${suffix}`;
}And when calling formatNftId, pass the NFT's creation timestamp (likely event.timestamp or a similar field) as the third argument.
Let me know if you need help updating the call sites or have any other questions!
|
|
||
| return `${wholePart}.${trimmedFractional}`; | ||
| } catch (error) { | ||
| return '0.758'; |
There was a problem hiding this comment.
suggestion (bug_risk): Hardcoded fallback value in formatAmount may be misleading.
Returning '0.758' on parse failure may mislead users. Use '0' or an explicit error indicator to avoid confusion.
| return '0.758'; | |
| return '0'; |
| @@ -0,0 +1,105 @@ | |||
| import { gql } from '@apollo/client'; | |||
There was a problem hiding this comment.
We have useSubgraphSteward already which contains most of this.
Just extend with any missing fields.
For example the claim-events as shared on TG:
collectives {
pooltype
collective {
claims {
events {
id
}
}
}
}
The ID of the event when poolType is: DirectPayments
holds the ipfs URI
There was a problem hiding this comment.
when i try extending the missing fields, the application breaks
There was a problem hiding this comment.
Have you verified the different fields available on: https://api.studio.thegraph.com/query/59211/goodcollective/dev-v1.0.13/graphql?query=query+MyQuery+%7B%0A++__typename+%23%23+Placeholder+value%0A%7D
see full query in TG dm
i made no change to deployment.json. It is most likely prettier formatting |
|
If no changes, please revert to previous version. it should only have changes upon deploying a new contract @EmekaManuel |
…prevent future formatting changes
i have reverted the changes @L03TJ3 |
| paymentAmount, | ||
| timestamp, | ||
| }: ActivityLogProps) { | ||
| const NFT_CA = '0x251EEBd7d9469bbcc02Ef23c95D902Cbb7fD73B3'; |
There was a problem hiding this comment.
This would not work hardcoded on all environments.
please read the approriate contract-address from the deployment.json.
see constants.ts
how to read from the deployment.json, and use the configured network (SupportedNetworkNames) to get the right contract based on network/env
| return `${wholePart}.${trimmedFractional}`; | ||
| } catch (error) { | ||
| return '0.758'; | ||
| console.warn('Error formatting amount:', error); |
There was a problem hiding this comment.
formatAmount is not needed.
You can just do Number(amount / 1e18).toFixed(3)
| } | ||
| }; | ||
|
|
||
| const formatDate = (dateString: string, timestampValue: number) => { |
There was a problem hiding this comment.
- methods like this should be helper methods outside of a hook (very likely to be re-used). now you have the same method in two places (Not following DRY principle)
- we put helper methods usually in /lib or /utils.
- in /lib, you will see there is already a formatTime helper. use that one
| function formatNftId(nftId: string, collectiveId: string): string { | ||
| const prefix = collectiveId.substring(0, 3).toUpperCase(); | ||
| const suffix = nftId.substring(nftId.length - 4); | ||
| const year = new Date().getFullYear(); | ||
| return `#${prefix}-${year}-${suffix}`; |
There was a problem hiding this comment.
True, nft's could have been created last year. please fix
| : undefined; | ||
|
|
||
| const { data: ensName } = useEnsName({ address, chainId: 1 }); | ||
| const fullName = useFetchFullName(address); |
There was a problem hiding this comment.
No need for fullname handling
| }, {} as Record<string, { name: string; count: number }>) | ||
| : {}; | ||
|
|
||
| const displayCollectives = (() => { |
There was a problem hiding this comment.
if this needs to be run on mount (seeing it being a IIFE).
use useEffect
…from the deployment.json file
| @@ -0,0 +1,11 @@ | |||
| export function formatAmount(amount: string): string { | |||
There was a problem hiding this comment.
There is no need for this complication.
Wherever you need to do this calculation just do Number(amount * 1e18).toFixed(3).
> amount = 500334455600000000000
500334455600000000000
> Number(amount / 1e18).toFixed(3)
'500.334'
| @@ -0,0 +1,7 @@ | |||
| export function formatDate(ts: number): string { | |||
There was a problem hiding this comment.
there is formatTime utility, no need for a new one
…ction for improved readability
…EmekaManuel/GoodCollective into td-221-implement-action-summary
…r accurate year representation
… display logic using useMemo and useEffect
… amount calculation for better precision
… improved timestamp formatting
…at for clarity and nft details link
2 participants
@sirpy @L03TJ3
#221
Description
This PR implements the Wallet Detail - Action Summary & Action Logs feature for stewards (#221). . The changes add comprehensive activity tracking and display functionality for steward wallets.
Key Features Added:
Technical Implementation:
How Has This Been Tested?
The changes have been tested through:
Files Changed:
Dependencies:
Description by Korbit AI
What change is being made?
Implement Activity Summary and Action Logs for Steward profiles in the wallet application.
Why are these changes being made?
These changes are introduced to provide stewards with a detailed view of their activities within their wallet profiles, enabling them to track and manage their environmental actions and transactions effectively. This is achieved through an enriched UI that includes the ability to expand and view detailed logs, as well as handling dynamic links to external resources such as blockchain explorers and IPFS. The solution leverages new hooks and utilities to seamlessly fetch and format data, while addressing potential UI and data transformation gaps.