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.

Sign up for GitHub

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

Jump to bottom

Collect tap ravs #869

Merged
merged 5 commits into from
May 9, 2024
Merged

Collect tap ravs #869

merged 5 commits into from
May 9, 2024

Conversation

carlosvdr
Copy link
Contributor

@carlosvdr carlosvdr commented Mar 14, 2024 •
edited
Loading

A continuation of PR 831
Were we fix redeeming for ravs, collection of allocations, unit tests, migrations among others

@gusinacio gusinacio mentioned this pull request Mar 14, 2024
Closed
@carlosvdr carlosvdr requested review from gusinacio and aasseman March 14, 2024 17:25
gusinacio
gusinacio reviewed Mar 14, 2024
View reviewed changes
Copy link
Member

@gusinacio gusinacio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first review, I still need to go through tests and query-fees.ts

packages/indexer-common/src/query-fees/object-conversion.ts Outdated Show resolved Hide resolved
packages/indexer-common/package.json Outdated Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/types.ts Outdated Show resolved Hide resolved
packages/indexer-service/src/allocations.ts Show resolved Hide resolved
aasseman
aasseman reviewed Mar 14, 2024
View reviewed changes
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/types.ts Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
aasseman
aasseman reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@aasseman aasseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This round is about the readability of pendingRAVs

packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Show resolved Hide resolved
gusinacio
gusinacio approved these changes Mar 19, 2024
View reviewed changes
Copy link
Member

@gusinacio gusinacio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@carlosvdr carlosvdr force-pushed the gusinacio/collect-tap-ravs branch 8 times, most recently from 7d5cdf1 to 9603446 Compare March 19, 2024 20:16
@aasseman aasseman removed their request for review March 21, 2024 17:00
aasseman
aasseman reviewed Mar 21, 2024
View reviewed changes
Copy link
Contributor

@aasseman aasseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for review by maintainers

@aasseman aasseman requested a review from fordN March 21, 2024 17:16
@aasseman aasseman marked this pull request as ready for review March 21, 2024 17:16
@aasseman aasseman self-requested a review March 21, 2024 17:17
@aasseman aasseman requested review from aasseman and gusinacio and removed request for aasseman and gusinacio March 21, 2024 17:19
gusinacio
gusinacio reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@gusinacio gusinacio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review post rebase

packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/query-fees/__test__/rav.test.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Show resolved Hide resolved
packages/indexer-common/src/query-fees/models.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/__tests__/tap.test.ts Outdated Show resolved Hide resolved
fordN
fordN reviewed Apr 9, 2024
View reviewed changes
Copy link
Contributor

@fordN fordN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite done reviewing everything, but figured it's worth submitting the comments I have now so we can start discussion.

packages/indexer-agent/src/__tests__/indexer.ts Show resolved Hide resolved
packages/indexer-agent/src/commands/start.ts Outdated Show resolved Hide resolved
packages/indexer-agent/src/db/migrations/12-add-scalar-tap-table.ts Show resolved Hide resolved
packages/indexer-agent/src/db/migrations/12-add-scalar-tap-table.ts Outdated Show resolved Hide resolved
packages/indexer-agent/src/db/migrations/12-add-scalar-tap-table.ts Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
packages/indexer-common/src/allocations/query-fees.ts Outdated Show resolved Hide resolved
@carlosvdr carlosvdr force-pushed the gusinacio/collect-tap-ravs branch from 2aa593c to bc82198 Compare April 22, 2024 22:09
@aasseman aasseman mentioned this pull request Apr 29, 2024
Closed
@carlosvdr carlosvdr mentioned this pull request May 2, 2024
Closed
fordN
fordN approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@fordN fordN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 😄

@carlosvdr carlosvdr force-pushed the gusinacio/collect-tap-ravs branch from 1d869b8 to 3f316fa Compare May 9, 2024 18:56
@carlosvdr carlosvdr force-pushed the gusinacio/collect-tap-ravs branch from 3f316fa to 13864f0 Compare May 9, 2024 19:09
@carlosvdr carlosvdr merged commit bc0b473 into main May 9, 2024
10 checks passed
@carlosvdr carlosvdr deleted the gusinacio/collect-tap-ravs branch May 9, 2024 19:28
@aasseman aasseman mentioned this pull request Jun 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkornuta-semiotic tkornuta-semiotic tkornuta-semiotic left review comments

@aasseman aasseman aasseman approved these changes

@gusinacio gusinacio gusinacio approved these changes

@Jannis Jannis Jannis left review comments

@fordN fordN fordN approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@carlosvdr @Jannis @fordN @gusinacio @aasseman @tkornuta-semiotic @hopeyen