refactor: staged writes in tagging stores #19476
Conversation
mverzilli
added
the
ci-no-fail-fast
mverzilli
changed the base branch from
next
to
martin/capsule-store-with-staged-writes
mverzilli
changed the title
AztecBot
force-pushed
the
martin/capsule-store-with-staged-writes
branch
from
1de3908 to
b30c796
Compare
nventuro
left a comment
nventuro
left a comment
There was a problem hiding this comment.
It looks like the capsule changes got here. Can you fix the branch?
benesjan
left a comment
benesjan
left a comment
There was a problem hiding this comment.
Most of it are nits and would approve if it were not for the sender_tagging_store test. Those seems to be quite messy.
All the naming suggestions are up for a debate. Just felt like plenty of it could be cuter.
BTW really like the whole staging approach you came up with. It's very elegant. Great job!
| end: number, | ||
| aztecNode: AztecNode, | ||
| taggingStore: SenderTaggingStore, | ||
| jobId: string, |
There was a problem hiding this comment.
The docs became stale here but I also feel like the param is not worth documenting (as well as aztecNode and taggingStore).
When I implemented loadPrivateLogsForSenderRecipientPair I decided to use the better style of docs which Nico proposed in Aztec.nr. Maybe we could do the same here.
| app: AztecAddress, | ||
| aztecNode: AztecNode, | ||
| taggingStore: SenderTaggingStore, | ||
| jobId: string, |
There was a problem hiding this comment.
Same as before here is also missing param desc but I see that I didn't bother with documenting here aztecNode and taggingStore when working on this.
Do you think it's ok to have the standard TSDoc style without having all the params described?
| * | ||
| * TODO(benesjan): Relocate to yarn-project/pxe/src/storage/tagging_store |
There was a problem hiding this comment.
| * | |
| * TODO(benesjan): Relocate to yarn-project/pxe/src/storage/tagging_store |
Unrelated but I forgot to drop this comment before and this is a good PR to sneak that in.
| return jobStagedHighestAgedIndex; | ||
| } | ||
|
|
||
| async #getHighestFinalizedIndexFromStage(jobId: string, secret: string): Promise<number | undefined> { |
There was a problem hiding this comment.
| async #getHighestFinalizedIndexFromStage(jobId: string, secret: string): Promise<number | undefined> { | |
| async #getHighestFinalizedIndexForJob(jobId: string, secret: string): Promise<number | undefined> { |
Think the original name is misleading (I think of stage as e.g. stake 2 of a multiple staged process). Please also update the rest if you agree.
| this.#stagedHighestFinalizedIndex = new Map(); | ||
| } | ||
|
|
||
| #getJobStagedHighestAgedIndex(jobId: string): Map<string, number> { |
There was a problem hiding this comment.
| #getJobStagedHighestAgedIndex(jobId: string): Map<string, number> { | |
| #getHighestAgedIndexForJob(jobId: string): Map<string, number> { |
Think this name is a bit easier to comprehend.
| const txHash1 = TxHash.random(); | ||
| const txHash2 = TxHash.random(); | ||
| const txHash3 = TxHash.random(); |
There was a problem hiding this comment.
These values are used only once so would drop these and have them be hardcoded directly in the call.
| const anotherStagingJobId: string = 'another-staging-job-id'; | ||
|
|
||
| // Committed: index 3 pending | ||
| await taggingStore.storePendingIndexes([{ secret: secret1, index: 3 }], txHash1, commitJobId); |
There was a problem hiding this comment.
OTOH the index here and below are then checked against so makes sense to have them be a constant.
|
|
||
| // Committed: index 3 pending | ||
| await taggingStore.storePendingIndexes([{ secret: secret1, index: 3 }], txHash1, commitJobId); | ||
| await taggingStore.commit(commitJobId); |
There was a problem hiding this comment.
Why even commit here? It should be fine to just check for undefined, no?
| expect(await taggingStore.getLastFinalizedIndex(secret1, stagingJobId)).toBe(3); | ||
| }); | ||
|
|
||
| it('stages pending and finalized index operations independently', async () => { |
There was a problem hiding this comment.
I feel like this test name could also be clearer. Why introduce "operations" here when the term is not used anywhere else?
| expect(await taggingStore.getLastUsedIndex(secret1, stagingJobId)).toBe(7); | ||
| }); | ||
|
|
||
| it('drops pending indexes in staging correctly', async () => { |
There was a problem hiding this comment.
This doesn't test anything new that removes all pending indexes for a given tx hash test case doesn't test.
Third part of the series started with #19445.
This makes the stores related to tagging synchronization work based on staged writes.