-
Notifications
You must be signed in to change notification settings - Fork 363
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
src/logic/safe/store/actions/transactions/fetchTransactions/index.ts
Outdated
Show resolved
Hide resolved
search
Some early feedback while the ticket is still in peer review. Safe for testing: Suggestion: Issue: 2 - The from/to filter is not working properly for outgoing tx. In the safe I filtering by outgoing tx, from May 2021 to Dec 2021 and the results still show tx from 2022. The payload only shows the "greater or equal than" parameter, but in the URL all the parameters are there 3 - Filtering by outgoing tx seems to filter only by those tx that send the main token of the network (ETH in this case) |
@franco, thanks for the early testing!
|
Nice! |
Yes, that's more appropriate IMHO. |
@katspaugh, I've updated it: |
In v2 we should make the Recipient field auto-complete from the Address Book, and the Token Address – from the hodled tokens. |
Both should autocomplete from the address book already. Are you testing on the PR? I like the idea of the token address from the asset list. We can add it to the list for when we enable date filtering. What do you think? I can, of course, implement it in this version though. |
Ah, I don't have an AB on there. Nice touch then! |
src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts
Show resolved
Hide resolved
src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts
Show resolved
Hide resolved
Safe used for testing: Some minor stuff The X shows in the recipient to clear the field, but after doing a filter it doesn't show again until you start to write something there or delete a character from the previous filter attempt. This wouldn't be such a hassle if selecting all the text and pressing backspace would not fail to delete what is written (but this is an issue present even in the contract interaction form) Major issue: If you search for only incoming with no amount you get a ton of results, many with "1's" in them: Why so many of those results are ignored when searching with the amount of 1 in the first try? |
I will look into this.
As it is "an issue present even in the contract interaction form", I would argue that this is out of scope. MUI adds the button as it is related to current user interaction so I personally think it's intended behaviour. However, I will look into this.
If you send funds to yourself, you are effectively creating an icoming and outgoing transaction. Afair, the gateway shows these only as incoming transactions. This was discussed with @johannesmoormann and, as it is a backend-related issue and an edge case (users won't be sending funds to themselves), it is fine.
We aren't converting the Findings to tackle
|
@francovenica, the I've also adjusted the clear button to always show when there is a value in the field and the text truncates if it goes beyond the button, as seen here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great feature, good work on this! 🚀
The issues reported (the X for the clear and searching for an amount of 1) where fixed. Nice Issues/Questions: Suggestion: We should probably think about a range for the amount input; if somebody sent you 123 USDC, unless you remember that exact number you are not going to find the tx Question/Suggestion: The "recipient" input for an incoming type of filter is skewing the result, since the only recipient for incoming tx is the safe itself, so the only value that it works is the safe address. We still have the issue of the "outgoing tx" filter only searching for the base token (ETH in rinkeby). I mentioned this in an older comment. Do we still need input from people from the backend about this? |
Thanks for the concise testing @francovenica!
I like the idea but this would require adjustments to the gateway. I've checked the swagger and both
Theorertically, you can filter by any Safe address. However, it makes total sense to not allow the user to do this on the frontend. I have removed the
In order to filter tokens, you need to include the token address in the filter. Please let me know about the removal of the recipient field. Hopefully we can get this merged. |
Ok, the input is gone from the incoming type of filtering I was told regarding the rest of the feedback it requires intervention from the backend. Looks good to me |
Just a follow up: I misunderstood the searching of tokens for outgoing transactions. It's incoming that has the I cannot see a query for it on the |
@iamacook The issue with the date filters has been resolved and it is live on |
What it solves
Requesting filtered transactions.
How this PR fixes it
The chosen filter is now applied to the historical transactions, as well as being appended to the URL (with the added benefit of being able to share).
Screenshots
How to test it
Filter the transactions and observe the filtered data. The filter should also be appended to the URL. Opening the filter-appended URL should load the historical transactions, filtered.