Add `is_source` query parameter to transaction endpoints #4035

2opremio · 2021-10-28T12:44:08Z

~~I'm happy to use a different name for the endpoint.~~

~~I used transactions_by_source to distinguish it from the existing endpoint GET /accounts/{account_id}/transactions~~

Closes #3688

2opremio · 2021-10-28T12:44:57Z

services/horizon/internal/db2/schema/migrations/51_transactions_by_source_account.sql

+DROP INDEX by_account;
+DROP INDEX by_fee_account;


@stellar/horizon-committers Please double check these indices aren't used (I couldn't find a reason to keep them)

by_account

I thought this would be used by GET /account/:account_id/transactions, but it looks like it joins/filters on the history_transaction_participants table, instead. Seems ok to remove, but is there a safer way to check that?

by_fee_account

I could've sworn I recalled either a PR or an issue that lets people filter fee-bump transactions by their fee source... can't find it now, maybe someone else remembers.

I haven't find any cases where the indexes are used. We can monitor the latencies in staging with mirroring on to verify there is no performance degradation

Shaptic

Seems good to me, minor notes below ⬇️

services/horizon/internal/httpx/router.go

Shaptic · 2021-11-02T21:04:24Z

services/horizon/internal/db2/schema/migrations/51_transactions_by_source_account.sql

+DROP INDEX by_account;
+DROP INDEX by_fee_account;


by_account

I thought this would be used by GET /account/:account_id/transactions, but it looks like it joins/filters on the history_transaction_participants table, instead. Seems ok to remove, but is there a safer way to check that?

by_fee_account

I could've sworn I recalled either a PR or an issue that lets people filter fee-bump transactions by their fee source... can't find it now, maybe someone else remembers.

services/horizon/internal/actions/transaction.go

2opremio · 2021-11-04T18:03:45Z

@bartekn @Shaptic PTAL

bartekn

LGTM!

bartekn · 2021-11-04T18:43:13Z

services/horizon/CHANGELOG.md

+### Changes
+
+* Add `is_source=true` parameter to transaction endpoints (e.g. `GET /accounts/{account_id}/transactions?is_source=true` ) to
+  allow filtering transactions based on their source account.


New line here a mistake?

The line was too long and it shouldn't make a difference in rendering.

bartekn · 2021-11-04T18:51:38Z

services/horizon/internal/db2/schema/migrations/51_transactions_by_source_account.sql

+DROP INDEX by_fee_account;
+
+-- recreate by_account index without the sequence, which will be used to query transactions by_source_account
+CREATE INDEX history_transactions_by_source_account ON history_transactions USING btree (account);


We already have multicolumn index which contains account:

"by_account" btree (account, account_sequence)

So I wonder if we need a new one. Postgres docs are not super clear on this but it seems queries should be fast if there are equality constraints on leading columns (so account in our case).

Why have an index on multiple columns if you only need it for one? (it would take less space)

Oh, we're removing it! I get it, sorry.

If creating the index will take 15 minutes, should we consider creating the index concurrently so that postgres doesn't lock the table during the 15 minutes? There are some caveats: https://www.postgresql.org/docs/9.1/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY

I used 15 minutes as an estimation, I didn't get to meassure it (we will find out when deploying to staging as part of testing)

leighmcculloch · 2021-11-05T14:07:16Z

services/horizon/CHANGELOG.md

+### Changes
+
+* Add `is_source=true` parameter to transaction endpoints (e.g. `GET /accounts/{account_id}/transactions?is_source=true` ) to
+  allow filtering transactions based on their source account.


Does this filter by transaction source account only, or operation source account too, and what about fee account? Is the goal to find transactions that only consume sequence numbers of the account (transaction source account), or transactions where the account chose to do something (transaction source account, operation source account, fee account). cc @tyvdh

Moved this discussion to #3688 (comment)

It filters by transaction source account only.

…is_source=true`

…llar#4035)" This reverts commit cecc89b.

…)" (#4049) This reverts commit cecc89b.

2opremio requested a review from a team October 28, 2021 12:44

2opremio commented Oct 28, 2021

View reviewed changes

2opremio force-pushed the 3688-add-transactions-by-source-endpoint branch from 955e1cb to ffb7dab Compare October 28, 2021 14:48

Shaptic approved these changes Nov 2, 2021

View reviewed changes

tamirms approved these changes Nov 4, 2021

View reviewed changes

2opremio changed the title ~~Add GET /accounts/{source_account_id}/transactions_by_source endpoint~~ Add is_source query parameter to transaction endpoints Nov 4, 2021

2opremio force-pushed the 3688-add-transactions-by-source-endpoint branch from a013fd1 to fe1bda6 Compare November 4, 2021 18:02

bartekn approved these changes Nov 4, 2021

View reviewed changes

leighmcculloch reviewed Nov 5, 2021

View reviewed changes

2opremio added 2 commits November 5, 2021 15:43

Add GET /accounts/{source_account_id}/transactions_by_source endpoint

cded7ad

Use query parameter instead `GET /accounts/{account_id}/transactions?…

d9ebee7

…is_source=true`

2opremio force-pushed the 3688-add-transactions-by-source-endpoint branch from fe1bda6 to f7066e1 Compare November 5, 2021 14:47

Update CHANGELOG

697945b

2opremio force-pushed the 3688-add-transactions-by-source-endpoint branch from f7066e1 to 697945b Compare November 5, 2021 14:49

tamirms mentioned this pull request Nov 5, 2021

Feature Request: Get Transactions for Account where Account is the Source lightsail-network/java-stellar-sdk#377

Closed

2opremio merged commit cecc89b into stellar:master Nov 5, 2021

leighmcculloch mentioned this pull request Nov 5, 2021

Feature Request: Get Transactions for Account where Account is the Source #3688

Closed

2opremio mentioned this pull request Nov 5, 2021

Revert "Add is_source query parameter to transaction endpoints" #4049

Merged

2opremio added a commit to 2opremio/go-2 that referenced this pull request Nov 5, 2021

Revert "Add is_source query parameter to transaction endpoints (ste…

223c8af

…llar#4035)" This reverts commit cecc89b.

2opremio added a commit that referenced this pull request Nov 5, 2021

Revert "Add is_source query parameter to transaction endpoints (#4035…

cc4b296

…)" (#4049) This reverts commit cecc89b.

2opremio deleted the 3688-add-transactions-by-source-endpoint branch November 10, 2021 11:53

2opremio restored the 3688-add-transactions-by-source-endpoint branch November 10, 2021 11:53

This was referenced Nov 17, 2021

services/horizon: Remove unused indexes from history_transactions #4085

Merged

Feature Request: Allow Efficient Access to Account "Actions" Where Account is Source #4051

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `is_source` query parameter to transaction endpoints #4035

Add `is_source` query parameter to transaction endpoints #4035

2opremio commented Oct 28, 2021 •

edited

Loading

2opremio Oct 28, 2021

Shaptic Nov 2, 2021

tamirms Nov 4, 2021

Shaptic left a comment

Shaptic Nov 2, 2021

2opremio commented Nov 4, 2021

bartekn left a comment

bartekn Nov 4, 2021

2opremio Nov 5, 2021

bartekn Nov 4, 2021

2opremio Nov 5, 2021

bartekn Nov 5, 2021

leighmcculloch Nov 5, 2021

2opremio Nov 5, 2021

leighmcculloch Nov 5, 2021

leighmcculloch Nov 5, 2021

2opremio Nov 5, 2021 •

edited

Loading

Add is_source query parameter to transaction endpoints #4035

Add is_source query parameter to transaction endpoints #4035

Conversation

2opremio commented Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shaptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2opremio commented Nov 4, 2021

bartekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2opremio Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Add `is_source` query parameter to transaction endpoints #4035

Add `is_source` query parameter to transaction endpoints #4035

2opremio commented Oct 28, 2021 •

edited

Loading

2opremio Nov 5, 2021 •

edited

Loading