Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Proxy Chain Filtering #4251

Open
wants to merge 6 commits into
base: feat/M2-proxy-colonies
Choose a base branch
from

Conversation

rumzledz
Copy link
Contributor

@rumzledz rumzledz commented Feb 5, 2025

Description

This PR only focuses on enabling Proxy Chain filtering for the Incoming, Balance & Activity pages.

During development, I have identified a couple of filter-related issues which are already present outside my PR. I have no intention of fixing these under this PR but I am more than happy to deal with them separately 😄

👉 CLICK TO SEE ALL ISSUES IDENTIFIED 👈

Issue 1: On mobile view, the Incoming page's search filter popup automatically renders when the filter popup is rendered

Screenshot 2025-02-05 at 14 36 46

Issue 2: When you deploy a Proxy Colony, you will naturally have a scenario where you have the same Token in both Colonies. This results in duplicate Token symbol entries due to how the Token filters components are set up:

Screenshot 2025-02-05 at 14 16 36

Issue 3: Ticking the checkbox for a non-unique Filter entry results in duplicate ticks:

image

Issue 4: The Incoming page's active filters list gets clipped when the viewport width is narrow:

Screenshot 2025-02-05 at 14 16 57

Issue 5: On the Activity page, filtering by the Split & Staged payment types seemingly does nothing due to how it's implemented 😅

Now that those are out of the way, here's Chain filtering in action:

Incoming page

incoming-chain-filter

Balances page

balance-chain-filters

Activity page

activity-chain-filter

Future refactor plans

I had some observations and found some challenges while implementing this which I intend to address in the future.

Note

These are just my thoughts of course.

👉 CLICK IF YOU'RE INTERESTED 👈

Filter Construct

Our filters are quite straightforward. They are mostly maps of string => boolean. Now while this is efficient, it doesn't allow for more flexibility when it comes to attaching metadata for a filter. For example, I wanted a Chain filter to carry both the label and the chain ID and I couldn't. As a result, I have to make a separate lookup to identify a Chain filter's corresponding Chain ID. I made some filter construct adjustments for the Balances page to make this more convenient for me:

filters = {
  chain: {
    [name of the filter]: {
      isChecked: boolean;
      id?: string; // Intended to be the Chain ID in this scenario but it could be any other thing tbh
    }
  },
  someOtherFilter: {
    [name of the filter]: {
      isChecked: boolean;
      id?: string;
    }
  }
}

This approach isn't perfect but it was the path of least resistance. I found that it required the most minimal adjustments to keep it compatible with our overall filter set up and at the same time, allow me the flexibility I needed. I want to come up with a better solution in the future but I'll be basing my future work on this concept.

Filter fragmentation

I found that we have multiple filter approach across our pages but most of them follow the same sort of structure. We tend to have multiple states to store filter chunks i.e.

const [tokenFilters, setTokenFilters] = useState();
const [otherFilter, setOtherFilter] = useState();

// dupe
const handleTokenFilterChange = (event) => {
  const isChecked = event.target.checked;
  const { name } = event.target;
  
  setTokenFilters(state => ({ ...state, [name]: isChecked }));
}

// dupe
const handleOtherFilterChange = (event) => {
  const isChecked = event.target.checked;
  const { name } = event.target;
  
  setOtherFilter(state => ({ ...state, [name]: isChecked }));
}

I came up with a unified approach for the Balance page filters as a POC but I made sure it didn't require me to make some major code-breaking changes across the board. I want to explore this concept further in the future.

const handleFiltersChange = (event, type) => {
  const { checked, name, id } = event.target;
  
  setFilters(state => ({
    ...state,
    [type]: {
      ...state[type],
      [name]: {
        isChecked: checked,
        id // I assigned the Chain ID to the id prop of the checkbox input
      }
    }
  });
}

// Usage
<input
  id={chainId}
  name={chainLabel}
  onChange={event => handleFiltersChange(event, 'chain')}
/>

We also have Filter components that can be unified and re-used across different pages but I understand why we have to duplicate them in our current setup.

Testing

Important

  1. Bring up the Manage supported chains form
  2. Add support for Local Proxy Chain 1 and submit the form
  1. Visit the Incoming page
  2. Filter the table by Chain -> Local Proxy Chain 1
  3. Verify that you can only see the ETH entry for Chain 265669101
  4. Visit the Balances page
  5. Filter the table by Chain -> Local Proxy Chain 1
  6. Well, this one is a bit awkward because there's not much to go by.. BUT, you should only see 1 ETH entry because the other hidden one is not from Chain 265669101
  7. Visit the Activity page

Warning

At the moment, if you filter by the main chain (Ganache in this instance), it won't work because actions have their targetChainId set to null by default. Our GraphQL is not set up to filter by null.

In fact, if you tried to filter by the Main Chain, you will get no results because the API itself returns an empty array.

The best recommendation was to perform a migration which pretty much checks if an action was made on the main chain, indicated by a null value for targetChainId and replaces it with the Main Chain ID.

This will be dealt with separately. So for now, please just try out Proxy chain filtering for the Activity page.

  1. Filter the table by Chain -> Local Proxy Chain 1
  2. Verify that you can see the Manage supported chains action you did earlier

Resolves #3461

@rumzledz rumzledz self-assigned this Feb 5, 2025
@rumzledz rumzledz marked this pull request as ready for review February 5, 2025 16:09
@rumzledz rumzledz requested a review from a team as a code owner February 5, 2025 16:09
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @rumzledz really nice start on this issue! 💯

Screenshot 2025-02-06 at 10 47 17

I haven't given it a proper review yet, but I noticed we display all proxy chain options even before the colony has deployed one, which should not be the case.
We'll need to make use however of the active proxy chains when filtering the chain options. So, we'll need something among these lines

  const activeProxyColoniesChainIds = useDeployedChainIds({
    filterFn: (deployedProxyColony) => deployedProxyColony?.isActive,
  });

  const chainOptions = useChainOptions({
    filterOptionsFn: (option) =>
      activeProxyColoniesChainIds?.includes(option.value.toString()) ||
      option.value === DEFAULT_NETWORK_INFO.chainId,
  });

Also, we'll need to add the default chain as an option to filter. You can have a look over @bassgeta's work here

@rumzledz rumzledz marked this pull request as draft February 6, 2025 10:05
@rumzledz rumzledz marked this pull request as ready for review February 7, 2025 13:53
@rumzledz rumzledz requested review from mmioana and a team February 7, 2025 14:11
@rumzledz rumzledz force-pushed the feat/3461-incoming-funds-chain-filtering branch from 42f9974 to 89a9a0d Compare February 7, 2025 14:23
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really fine work once again @rumzledz 🎉

Before deploying a proxy colony the chain options show properly for the Manage supported chains action. Tested also after deploying if the options update properly
Screenshot 2025-02-07 at 15 19 54
Screenshot 2025-02-07 at 15 23 51
Screenshot 2025-02-07 at 15 23 58

And all the filters show up very nicely 🥇

Screenshot 2025-02-07 at 15 20 44
Screenshot 2025-02-07 at 15 21 08
Screenshot 2025-02-07 at 15 21 20
Screenshot 2025-02-07 at 15 21 28
Screenshot 2025-02-07 at 15 21 35
Screenshot 2025-02-07 at 15 21 43

Disabled a proxy colony and the proxy chain is no longer among the filtering options
Screenshot 2025-02-07 at 15 25 37
Screenshot 2025-02-07 at 15 25 19
Screenshot 2025-02-07 at 15 25 27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants