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

fix: support sender signers #110

Merged
merged 6 commits into from
Jan 17, 2024
Merged

fix: support sender signers #110

merged 6 commits into from
Jan 17, 2024

Conversation

aasseman
Copy link
Contributor

The TAP implementation was assuming that the receipts/RAVs are signed by the Sender (aka. Escrow account owner), but they are actually signed by one of the signers (a key specifically created only for signing receipts/RAVs in the sender's name).
A sender can have any number of valid/thawing/expired signers.

This makes the code even uglier... I hope that I can think of a cleaner architecture later.

@aasseman aasseman added size:large Large p0 Critical priority type:bug Something isn't working labels Jan 13, 2024
@aasseman aasseman self-assigned this Jan 13, 2024
Copy link

github-actions bot commented Jan 13, 2024

Pull Request Test Coverage Report for Build 7548678988

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 62.404%

Totals Coverage Status
Change from base Build 7469315487: 0.1%
Covered Lines: 2362
Relevant Lines: 3785

💛 - Coveralls

Signed-off-by: Alexis Asseman <[email protected]>
Copy link
Collaborator

@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.

Great work, I have a few questions about how the contract works, a small suggestion with naming, and another one to remove an unused variable declaration.

common/src/escrow_accounts.rs Outdated Show resolved Hide resolved
common/src/escrow_accounts.rs Outdated Show resolved Hide resolved
common/src/escrow_accounts.rs Outdated Show resolved Hide resolved
tap-agent/src/tap/escrow_adapter.rs Outdated Show resolved Hide resolved
common/src/tap_manager.rs Outdated Show resolved Hide resolved
tap-agent/src/tap/escrow_adapter.rs Outdated Show resolved Hide resolved
common/src/escrow_accounts.rs Outdated Show resolved Hide resolved
tap-agent/src/tap/receipt_checks_adapter.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Beautiful code. LGTM

@aasseman aasseman merged commit 36a73a9 into main Jan 17, 2024
8 checks passed
@aasseman aasseman deleted the aasseman/signers_support branch January 17, 2024 00:39
aasseman added a commit that referenced this pull request Jan 17, 2024
aasseman added a commit that referenced this pull request Jan 23, 2024
* feat(indexer-service): Sender denylist

indexer-service will reject all paid queries from denied senders.
Denylist DB table updates are watched through PG notifications.

Signed-off-by: Alexis Asseman <[email protected]>

* refactor: cleaner sender_denylist_watcher shutdown

Signed-off-by: Alexis Asseman <[email protected]>

* refactor: remove unnecessary Arc on CancellationToken

Signed-off-by: Alexis Asseman <[email protected]>

* fix: proper signers support after #110

Signed-off-by: Alexis Asseman <[email protected]>

---------

Signed-off-by: Alexis Asseman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p0 Critical priority size:large Large type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants