refactor!: splitting getLogsByTags to private and public versions
#19158
+727
−410
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 19, 2025
Contributor
Author
benesjan
force-pushed
the
12-19-refactor_typing_tag_arg_in_getlogsbytags
branch
2 times, most recently
from
a36eb2f to
1a47f83
Compare
benesjan
commented
Dec 19, 2025
yarn-project/pxe/src/contract_function_simulator/pxe_oracle_interface.ts
Outdated
Show resolved
Hide resolved
benesjan
changed the title
benesjan
added
the
ci-no-fail-fast
mverzilli
approved these changes
Dec 22, 2025
mverzilli
approved these changes
Dec 22, 2025
spalladino
approved these changes
Dec 22, 2025
benesjan
changed the base branch from
12-19-feat_optimizing_aztecnode_getlogsbytags_for_new_log_sync_algo
to
graphite-base/19158
benesjan
force-pushed
the
12-19-refactor_typing_tag_arg_in_getlogsbytags
branch
from
7f81d21 to
e225434
Compare
benesjan
force-pushed
the
graphite-base/19158
branch
from
528b8ae to
19b00c9
Compare
benesjan
force-pushed
the
12-19-refactor_typing_tag_arg_in_getlogsbytags
branch
from
e225434 to
8b2772a
Compare
benesjan
commented
Dec 22, 2025
benesjan
commented
Dec 22, 2025
benesjan
changed the title
getLogsByTags to private and public versions
benesjan
changed the title
getLogsByTags to private and public versionsgetLogsByTags to private and public versions
benesjan
force-pushed
the
12-19-refactor_typing_tag_arg_in_getlogsbytags
branch
from
8b2772a to
cd892e9
Compare
benesjan
commented
Dec 22, 2025
yarn-project/pxe/src/contract_function_simulator/noir-structs/log_retrieval_request.ts
Show resolved
Hide resolved
benesjan
commented
Dec 22, 2025
yarn-project/pxe/src/tagging/recipient_sync/utils/load_logs_for_range.test.ts
Show resolved
Hide resolved
benesjan
commented
Dec 22, 2025
mverzilli
approved these changes
Dec 23, 2025
Contributor
mverzilli
left a comment
mverzilli
left a comment
There was a problem hiding this comment.
Approving because it's overall good and I won't be around to approve later, but we probably should also add migration notes, shouldn't we?
yarn-project/archiver/src/archiver/kv_archiver_store/log_store.ts
Outdated
Show resolved
Hide resolved
yarn-project/archiver/src/archiver/kv_archiver_store/log_store.ts
Outdated
Show resolved
Hide resolved
yarn-project/archiver/src/archiver/kv_archiver_store/log_store.ts
Outdated
Show resolved
Hide resolved
| const txEffect = await TxEffect.random(); | ||
| txEffect.privateLogs = mockPrivateLogs(blockNumber, txIndex); | ||
| txEffect.publicLogs = mockPublicLogs(blockNumber, txIndex); | ||
| txEffect.publicLogs = []; // No public logs needed for private log tests |
Contributor
There was a problem hiding this comment.
Shouldn't we test some cases with public logs? Even when we know the implementation handles it well now, we probably want to spec that, especially consider the LogStore processes and reduces logs of both types at the same time
Contributor
Author
There was a problem hiding this comment.
There is a new block of tests for getPublicLogsByTagsFromContract. Or do you think they are insufficient.
spalladino
approved these changes
Dec 23, 2025
Contributor
There was a problem hiding this comment.
Archiver changes look good to me, and agree with @mverzilli on extracting some helpers to make it easier to follow. I also agree with him in that you are a really nice person.
benesjan
force-pushed
the
12-19-refactor_typing_tag_arg_in_getlogsbytags
branch
from
6da68b6 to
0d69348
Compare
Contributor
Author
Indeed, I have some more changes to this endpoint in the pipeline and I plan on writing them once I am done with those. But thanks for reminding me |
benesjan
added
ci-squash-and-merge
and removed
ci-no-fail-fast
benesjan
force-pushed
the
12-19-refactor_typing_tag_arg_in_getlogsbytags
branch
2 times, most recently
from
859acfe to
f88fa5c
Compare
Fixes #14555 ## New PR Description As mentioned in the old PR description I originally planned to just type the arg of `getLogsByTags` but while addressing this PR's feedback I realized that I made a mistake: the `getLogsByTags` endpoint was used for both private and public logs and the issue is that for public logs we use "unsiloed" tag and for private logs we use siloed tag. Having these 2 endpoints merged was just a tech debt: there is not a place where we would call this endpoint expecting only 1 type of logs. This led to us having ugly utilities that filtered out one kind of logs after retrieval. We didn't split this before because we didn't want to introduce a breaking change to the API. Given that I am already breaking backwards compatibility and given the error I made it made sense to fix this by splitting the two endpoints. Now we have `getPrivateLogsByTag` and `getPublicLogsByTagsFromContract` endpoints. ## Old PR Description In this PR I used the `SiloedTag` type for the param in `getLogsByTags`. This allowed me to drop some ugly type conversions. This was just a tech debt. The diff turned out to be quite large because I needed to move the type from `pxe` to `stdlib`. As mentioned in a PR down the stack this is second of 3 PRs in which I clean up the `getLogsByTags` endpoint: 1. First PR - including block timestamp in return value, 2. This is the second PR - I type the tag arg of the function to be `SiloedTag`, 3. in the last PR I will drop pagination from this endpoint. Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
AztecBot
force-pushed
the
12-19-refactor_typing_tag_arg_in_getlogsbytags
branch
from
f88fa5c to
e8de31f
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Dec 23, 2025
…19158) Fixes #14555 ## New PR Description As mentioned in the old PR description I originally planned to just type the arg of `getLogsByTags` but while addressing this PR's feedback I realized that I made a mistake: the `getLogsByTags` endpoint was used for both private and public logs and the issue is that for public logs we use "unsiloed" tag and for private logs we use siloed tag. Having these 2 endpoints merged was just a tech debt: there is not a place where we would call this endpoint expecting only 1 type of logs. This led to us having ugly utilities that filtered out one kind of logs after retrieval. We didn't split this before because we didn't want to introduce a breaking change to the API. Given that I am already breaking backwards compatibility and given the error I made it made sense to fix this by splitting the two endpoints. Now we have `getPrivateLogsByTag` and `getPublicLogsByTagsFromContract` endpoints. ## Old PR Description In this PR I used the `SiloedTag` type for the param in `getLogsByTags`. This allowed me to drop some ugly type conversions. This was just a tech debt. The diff turned out to be quite large because I needed to move the type from `pxe` to `stdlib`. As mentioned in a PR down the stack this is second of 3 PRs in which I clean up the `getLogsByTags` endpoint: 1. First PR - including block timestamp in return value, 2. This is the second PR - I type the tag arg of the function to be `SiloedTag`, 3. in the last PR I will drop pagination from this endpoint.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #14555
New PR Description
As mentioned in the old PR description I originally planned to just type the arg of
getLogsByTagsbut while addressing this PR's feedback I realized that I made a mistake: thegetLogsByTagsendpoint was used for both private and public logs and the issue is that for public logs we use "unsiloed" tag and for private logs we use siloed tag.Having these 2 endpoints merged was just a tech debt: there is not a place where we would call this endpoint expecting only 1 type of logs. This led to us having ugly utilities that filtered out one kind of logs after retrieval.
We didn't split this before because we didn't want to introduce a breaking change to the API.
Given that I am already breaking backwards compatibility and given the error I made it made sense to fix this by splitting the two endpoints.
Now we have
getPrivateLogsByTagandgetPublicLogsByTagsFromContractendpoints.Old PR Description
In this PR I used the
SiloedTagtype for the param ingetLogsByTags. This allowed me to drop some ugly type conversions. This was just a tech debt.The diff turned out to be quite large because I needed to move the type from
pxetostdlib.As mentioned in a PR down the stack this is second of 3 PRs in which I clean up the
getLogsByTagsendpoint:SiloedTag,