grant_installation #50

tsachiherman · 2024-01-29T17:23:49Z

What ?

This PR adds support for registering a new installation.

Design considerations

The xps-gateway is the executable endpoint, that manages the rpc endpoint. It will setup an rpc server that would listen for incoming requests and perform input validation. Valid requests would be forwarded to the inbox / messaging / registry crates for method-specific implementation.

Requirements

This PR followed the design details in #22; discussion in #13 and xmtp/libxmtp#274

insipx · 2024-01-29T17:26:44Z

should have an integration test in the style of revoke_installation

codecov · 2024-01-29T17:27:29Z

Codecov Report

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

Comparison is base (1439a2e) 77.32% compared to head (fff0803) 82.01%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
registry/src/lib.rs	94.59%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   77.32%   82.01%   +4.69%     
==========================================
  Files          10       10              
  Lines         172      228      +56     
==========================================
+ Hits          133      187      +54     
- Misses         39       41       +2

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

gateway-types/src/lib.rs

jac18281828 · 2024-01-29T18:26:46Z

registry/src/lib.rs

+        let attribute: [u8; 32] = Attribute::from(name).into();
+        log::debug!(
+            "setting attribute {:#?}",
+            String::from_utf8_lossy(&attribute)


Remember that this string will always be created, even if this log is suppressed. This may be slightly too verbose logging.

I'm perfectly fine eliminating that. it was used during my debugging, but I don't feel strongly here.

tsachiherman · 2024-01-29T18:32:47Z

@jac18281828 question - I've calculated a validity to be one year in the future.
The issue is that it needs to be exactly the same validity for the signer and the sender.
so, taking current_block + 1 year might change. two options:

add the validity to the grant_installation rpc arguments.
set the validity so that it won't change between signing and sending. for example - trim it to a 1000's round a year from n ow. i.e. validity = (block_number + 1 year ) & 0xfffffff000

insipx · 2024-01-29T18:36:15Z

registry/src/lib.rs

+            )
+            .send()
+            .await?
+            .await?;


so far, we've been doing this strategy with .await?.await? which first sends the transaction, gets back a PendingTransaction, and then waits for it to be included + a specified amount of confirmations (default of 1). I like what you did below, though, but unsure what the better path would be:

Do we make the gateway wait until tx is confirmed or do we just return the tx_hash and have libxmtp/client wait?

We can instead just return the txHash from the RPC, and not wait on transaction inclusion, leaving tx confirmations up to the client. We do give up some control if something goes wrong.

Not a super important detail, but maybe worth a quick discussion WDYT @tsachiherman @jac18281828 @37ng

I think that it's a very important observation, as this is a blocking operation that consumes servers memory for the entire duration of the operation.
Another point is that regardless if we make it "non-blocking" or not, we need to be able to detect a fail cases so that we can log these.

That's true, IMO I think we should hand off control of the transaction to the client and just return txHash. A PendingTransaction can be easily rebuilt from libxmtp side by just providing the hash and a provider, so if anything fails they could re-submit the transaction

We could also provide an endpoint to do this for them, i.e subscribeTransaction or something along those lines

We should avoid waiting whenever possible in the service. We haven't spent much time optimizing the number of threads, etc., these threads will be precious.

made an issue for this: #52

the return of revoke_installation will have to change as well

insipx · 2024-01-29T18:39:15Z

registry/src/lib.rs

@@ -23,16 +24,57 @@ where
        Self { registry }
    }

+    fn resolve_did_address(&self, did: String) -> Result<H160, ContactOperationError<M>> {
+        // for now, we will just assume the DID is a valid ethereum wallet address
+        // TODO: Parse or resolve the actual DID


Made into an issue, #51

insipx · 2024-01-29T18:39:22Z

registry/src/lib.rs

@@ -23,16 +24,57 @@ where
        Self { registry }
    }

+    fn resolve_did_address(&self, did: String) -> Result<H160, ContactOperationError<M>> {
+        // for now, we will just assume the DID is a valid ethereum wallet address
+        // TODO: Parse or resolve the actual DID


Suggested change

// TODO: Parse or resolve the actual DID

insipx

Looks good! I'm willing to approve after an integration test is added

jac18281828 · 2024-01-29T18:41:37Z

@jac18281828 question - I've calculated a validity to be one year in the future. The issue is that it needs to be exactly the same validity for the signer and the sender. so, taking current_block + 1 year might change. two options:

add the validity to the grant_installation rpc arguments.

set the validity so that it won't change between signing and sending. for example - trim it to a 1000's round a year from n ow. i.e. validity = (block_number + 1 year ) & 0xfffffff000

The validity period should be sent to the contract (how long). The contract computes the validity internally according to the EVM block timestamp and reports back the validity that it came up with. That validity should then be used in the future for all operations. The validity required for the signature will be the duration validity (one year) not the actual timestamp.

We may want to standardize on two options to begin with: does not expire, or one year

tsachiherman · 2024-01-29T18:42:43Z

Looks good! I'm willing to approve after an integration test is added

see test_grant_installation()

insipx · 2024-01-29T18:47:20Z

We could do this in the style of the attribute, i.e an enum like

enum Validity {
    OneYear
    Forever
}

jac18281828 · 2024-01-29T18:50:12Z

We could do this in the style of the attribute, i.e an enum like
enum Validity {
    OneYear
    Forever
}

Yeah, my bad, I should have looked it up before I wrote my message but a validity is always required. We may want to choose either OneYear or FiveYear.

tsachiherman · 2024-01-29T20:50:01Z

I'll merge it and keep working on the extended unit tests & validity.

tsachiherman added 3 commits January 29, 2024 10:11

update

199eab9

update

1b27f34

update

7286f1a

tsachiherman requested a review from insipx January 29, 2024 17:23

tsachiherman self-assigned this Jan 29, 2024

update

fff0803

jac18281828 reviewed Jan 29, 2024

View reviewed changes

insipx reviewed Jan 29, 2024

View reviewed changes

insipx mentioned this pull request Jan 29, 2024

Feature request: Return Transaction Hash instead of waiting for Tx Inclusion #52

Open

insipx approved these changes Jan 29, 2024

View reviewed changes

tsachiherman merged commit d5fc1cd into xmtp:main Jan 29, 2024
4 of 5 checks passed

tsachiherman deleted the tsachi/add_installation branch January 29, 2024 20:50

insipx mentioned this pull request Feb 2, 2024

Contact Operations – Registry – RegisterInstallation #22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grant_installation #50

grant_installation #50

tsachiherman commented Jan 29, 2024

insipx commented Jan 29, 2024

codecov bot commented Jan 29, 2024 •

edited

Loading

jac18281828 Jan 29, 2024

tsachiherman Jan 29, 2024

tsachiherman commented Jan 29, 2024

insipx Jan 29, 2024

tsachiherman Jan 29, 2024

insipx Jan 29, 2024 •

edited

Loading

jac18281828 Jan 29, 2024

insipx Jan 29, 2024

insipx Jan 29, 2024

insipx Jan 29, 2024

insipx left a comment •

edited

Loading

jac18281828 commented Jan 29, 2024

tsachiherman commented Jan 29, 2024 •

edited by insipx

Loading

insipx commented Jan 29, 2024

jac18281828 commented Jan 29, 2024

tsachiherman commented Jan 29, 2024

grant_installation #50

grant_installation #50

Conversation

tsachiherman commented Jan 29, 2024

What ?

Design considerations

Requirements

insipx commented Jan 29, 2024

codecov bot commented Jan 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsachiherman commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insipx Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

insipx left a comment • edited Loading

Choose a reason for hiding this comment

jac18281828 commented Jan 29, 2024

tsachiherman commented Jan 29, 2024 • edited by insipx Loading

insipx commented Jan 29, 2024

jac18281828 commented Jan 29, 2024

tsachiherman commented Jan 29, 2024

codecov bot commented Jan 29, 2024 •

edited

Loading

insipx Jan 29, 2024 •

edited

Loading

insipx left a comment •

edited

Loading

tsachiherman commented Jan 29, 2024 •

edited by insipx

Loading