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

Revoke Installation #38

Merged
merged 23 commits into from
Jan 26, 2024
Merged

Revoke Installation #38

merged 23 commits into from
Jan 26, 2024

Conversation

insipx
Copy link
Contributor

@insipx insipx commented Jan 19, 2024

Closes #21

Has a companion xmtp/didethresolver#35 , to add the logic of signing attributes/delegates/owners

changes the name parameter of the RPC to a XmtpAttribute, which is able to convert between the string representation of the attribute ("xmtp/installation/hex") and the byte [u8; 32] representation, which is taken directly from lib_didethresolver so that if the library type changes, it will be evident in the gateway via compilation error.

Other misc changes:

  • adds anvil for integration testing (required to revoke attributes w/ eth node)
  • only runs lib and doctests for gh pages
  • modifies dockerfile as necessary
  • runs coverage with trace logs, so we don't get missing coverage errors for those logs anymore

Copy link
Contributor

@jac18281828 jac18281828 left a comment

Choose a reason for hiding this comment

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

Looks like a good start!

I think the key type makes sense. I think there may be some missed opportunities to add test coverage here too.

@insipx
Copy link
Contributor Author

insipx commented Jan 22, 2024

Looks like a good start!

I think the key type makes sense. I think there may be some missed opportunities to add test coverage here too.

yeah definitely still missing tests!

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (ce75da2) 78.94% compared to head (1536bf5) 77.32%.

Files Patch % Lines
xps-gateway/src/rpc/methods.rs 66.66% 8 Missing ⚠️
xps-gateway/src/lib.rs 0.00% 7 Missing ⚠️
registry/src/lib.rs 92.59% 2 Missing ⚠️
registry/src/error.rs 0.00% 1 Missing ⚠️
xps-gateway/src/types.rs 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
- Coverage   78.94%   77.32%   -1.63%     
==========================================
  Files           9       10       +1     
  Lines         114      172      +58     
==========================================
+ Hits           90      133      +43     
- Misses         24       39      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@insipx insipx marked this pull request as ready for review January 25, 2024 19:31
@tsachiherman tsachiherman self-requested a review January 26, 2024 21:43
@insipx insipx merged commit 1439a2e into main Jan 26, 2024
3 of 5 checks passed
@insipx insipx deleted the insipx/revoke-installation branch January 26, 2024 21:59
Copy link
Contributor

@jac18281828 jac18281828 left a comment

Choose a reason for hiding this comment

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

This all looks good. Sorry it took me so long to get back to this as I was fighting some integration test issues today. It turns out that this change breaks the production deployment. I think that is in part my fault since I didn't add the production build to the ci.

I'll create a ticket and assign it to you.

/// * `name` - the name of the contact bundle variant
/// * `value` - the value of the contact bundle
/// * `signature` - the signature of the contact bundle
#[method(name = "revokeInstallation")]
Copy link
Contributor

Choose a reason for hiding this comment

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

🚀

pub const SERVER_HOST: &str = "127.0.0.1";

#[cfg(test)]
mod it {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change loses the it module? That is how I am excluding the anvil tests in the pages build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I can add it back, I was just adding integration tests that were longer, and wanted to remove some of the indentation, didn't realize it was tied to the gh pages build

@insipx insipx mentioned this pull request Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Contact Operations – Registry – RevokeInstallation
3 participants