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: Get transactions #9

Merged
merged 11 commits into from
Sep 13, 2024
Merged

feat: Get transactions #9

merged 11 commits into from
Sep 13, 2024

Conversation

tmjssz
Copy link
Contributor

@tmjssz tmjssz commented Aug 21, 2024

This PR adds the following hooks which are wrapped by the top-level useSafe() hook.

Resolves #4.

usePendingTransactions

Fetches all pending multisig transactions for the connected Safe which have not been executed on-chain yet.

useTransactions

Fetches all transactions for the connected Safe which have been executed on-chain.

useTransaction

Fetches the status of a specific transaction by a Safe transaction hash or an Ethereum transaction hash.

Demo

To see the new hooks in action, check out the corresponding demo app PR.

@coveralls
Copy link

coveralls commented Aug 21, 2024

Pull Request Test Coverage Report for Build 10828383584

Details

  • 53 of 55 (96.36%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 95.263%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/hooks/usePendingTransactions.ts 13 14 92.86%
src/hooks/useSafeTransaction.ts 10 11 90.91%
Totals Coverage Status
Change from base Build 10474849315: -0.6%
Covered Lines: 311
Relevant Lines: 319

💛 - Coveralls

@tmjssz tmjssz marked this pull request as draft August 21, 2024 16:30
@tmjssz tmjssz marked this pull request as ready for review September 11, 2024 11:39
@yagopv
Copy link
Member

yagopv commented Sep 12, 2024

Why not to have only 1 hook useTransactions with an optional parameter safeTxHash instead 2?

@yagopv yagopv self-requested a review September 12, 2024 13:34
@tmjssz
Copy link
Contributor Author

tmjssz commented Sep 13, 2024

Why not to have only 1 hook useTransactions with an optional parameter safeTxHash instead 2?

You mean, combining useTransactions and useTransaction?
To me it's just more clear what the specific hooks do when having them separated. Because useTransactions returns only the executed transactions, while useTransaction can fetch a pending multisig transaction which is not executed yet -> so I see them as different use cases.
If we had them combined in one hook, I think it might be harder to understand how to use it (which params are needed, what does it return). But open to suggestions of course.

@tmjssz tmjssz merged commit b42bf34 into main Sep 13, 2024
1 check passed
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.

[React Hooks] Get transaction hooks
4 participants