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

feat: rs-sdk: rust software development kit for Dash Platform #1208

Closed
wants to merge 75 commits into from

Conversation

lklimek
Copy link
Contributor

@lklimek lklimek commented Jun 28, 2023

Issue being fixed or feature implemented

This PR implements multiple elements needed to build Rust SDK for Dash Platform.

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@lklimek lklimek self-assigned this Jun 28, 2023
// Now, lookup quorum details
let chain_id = mtd.chain_id.clone();
let quorum_type = proof.quorum_type;
let pubkey_bytes = provider.get_quorum_public_key(quorum_type, quorum_hash.to_vec())?;
Copy link
Member

Choose a reason for hiding this comment

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

we should also pass the core height to verify that the quorum is still active at that height.

// #[cfg(feature = "mockall")]

/// Create an object based on proof received from DAPI
pub trait FromProof<Req, Resp> {
Copy link
Contributor

@fominok fominok Jul 3, 2023

Choose a reason for hiding this comment

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

These parameters made as generics imply that there are possibility to have more than one implementation of the trait for a type, but is it our case? If not, it's better to have them as associated types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In platform.proto, we have at least two services that return Identity:

  rpc getIdentity (GetIdentityRequest) returns (GetIdentityResponse);
  rpc getIdentityByPublicKeyHashes (GetIdentityByPublicKeyHashesRequest) returns (GetIdentityByPublicKeyHashesResponse);

So the answer is that we can have more than one.

Copy link
Contributor

@fominok fominok Jul 14, 2023

Choose a reason for hiding this comment

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

as I see in this snippet they both return their own response types

Copy link
Contributor

@fominok fominok Jul 14, 2023

Choose a reason for hiding this comment

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

I mean yes, you're right there will be more than one FromProof implementation for Identity, but for each of implementation a pair of Req:Resp is fixed, so only one of them is a generic parameter

edit: I may sound confusing, here is tldr: make Resp an associated type instead of trait's generic parameter, so FromProof<Req> will have an exactly one type Resp

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with fominok (I think)

/// It defines a single method `get_quorum_public_key` which retrieves the public key of a given quorum.
#[uniffi::export(callback_interface)]
#[cfg_attr(feature = "mock", mockall::automock)]
pub trait QuorumInfoProvider: Send + Sync {
Copy link
Contributor

@fominok fominok Jul 4, 2023

Choose a reason for hiding this comment

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

We have trait and callback interface just to get a quorum public key that might be just passed as a parameter, how sure we are that this trait will grow? If so, do we really need to invert how dependencies are taken instead of having each method to have their own requirements listed as arguments?

I just don't get this whole dynamic story with vague provider instead of statically known calls with their own dependencies (or put parameters inside request objects).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea behind design of this API is a kind of "proxy"/"middleware" that gets request and response, and returns object retrieved from the proof inside the response. So, I expect the user (eg. third-party developer) to not understand - and not even try to read - the response. The intention here is to just translate received response from gRPC transport layer to DPP business objects, with as small amount of understanding of the underlying logic as possible.

Unfortunately, there is a missing piece needed to achieve that - public key. So I followed an approach similar to - for example - rust tls's x.509 certificate resolver.

Some other considerations:

  • the user does not know quorum hash upfront - so he cannot provide quorum public key without inspecting response,
  • public key can be either stored inside user's app, or looked up on an as-needed basis.

If we agree on going with callback design, we use a trait to implement it, as described in uniffi docs.

fn get_quorum_public_key(
&self,
quorum_type: u32,
quorum_hash: Vec<u8>,
Copy link
Member

Choose a reason for hiding this comment

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

quorum hash should be [u8;32]

&self,
quorum_type: u32,
quorum_hash: Vec<u8>,
) -> Result<Vec<u8>, Error>;
Copy link
Member

Choose a reason for hiding this comment

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

should return [u8;48]

jawid-h and others added 26 commits August 8, 2023 13:13
@lklimek lklimek changed the title feat: light client with proof verification and bindings feat: rs-sdk: rust software development kit for Dash Platform Oct 4, 2023
@shumkov shumkov deleted the branch v0.25-dev October 10, 2023 14:25
@shumkov shumkov closed this Oct 10, 2023
@lklimek
Copy link
Contributor Author

lklimek commented Oct 10, 2023

Closed by mistake, recreated as #1475

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants