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

add dips graphql #331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add dips graphql #331

wants to merge 1 commit into from

Conversation

mangas
Copy link

@mangas mangas commented Sep 30, 2024

No description provided.

@mangas mangas force-pushed the dips-graphql branch 2 times, most recently from 1c724c9 to ab06867 Compare September 30, 2024 15:20
@coveralls
Copy link

coveralls commented Sep 30, 2024

Pull Request Test Coverage Report for Build 11443538911

Details

  • 306 of 508 (60.24%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.8%) to 70.315%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/src/indexer_service/http/config.rs 0 1 0.0%
service/src/config.rs 0 4 0.0%
service/src/database/dips.rs 5 13 38.46%
service/src/service.rs 0 27 0.0%
dips/src/lib.rs 227 292 77.74%
service/src/routes/dips.rs 73 170 42.94%
Totals Coverage Status
Change from base Build 11409083574: -0.8%
Covered Lines: 4960
Relevant Lines: 7054

💛 - Coveralls

@mangas mangas force-pushed the dips-graphql branch 3 times, most recently from 2c3a2ec to 4a514e9 Compare October 2, 2024 13:47
@mangas mangas requested review from fubhy and LNSD and removed request for fubhy October 2, 2024 15:31
@mangas mangas marked this pull request as draft October 8, 2024 10:26
@mangas mangas force-pushed the dips-graphql branch 6 times, most recently from 72ebe2c to e2359ab Compare October 8, 2024 11:05
Copy link
Contributor

@LNSD LNSD left a comment

Choose a reason for hiding this comment

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

Please, check my comments 🙂

pub expected_payee: String,
pub allowed_payers: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the thegraph_core::Address type here, as we'll provide Ethereum addresses. This way, we can validate at deserialization time.

Comment on lines 311 to 371
#[derive(Debug, Deserialize)]
#[cfg_attr(test, derive(PartialEq))]
pub struct DipsConfig {
pub expected_payee: String,
pub allowed_payers: Vec<String>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here

dips/src/lib.rs Outdated
Comment on lines 16 to 76
sol! {
// EIP712 encoded bytes, ABI - ethers
#[derive(Debug, RlpEncodable, RlpDecodable, PartialEq)]
struct SignedIndexingAgreementVoucher {
IndexingAgreementVoucher voucher;
bytes signature;
}

#[derive(Debug, RlpEncodable, RlpDecodable, PartialEq)]
struct IndexingAgreementVoucher {
// should coincide with signer
address payer;
// should coincide with indexer
address payee;
// data service that will initiate payment collection
address service;
// initial indexing amount max
uint256 maxInitialAmount;
uint256 maxOngoingAmountPerEpoch;
// time to accept the agreement, intended to be on the order
// of hours or mins
uint64 deadline;
uint32 maxEpochsPerCollection;
uint32 minEpochsPerCollection;
// after which the agreement is complete
uint32 durationEpochs;
bytes metadata;
}

// the vouchers are generic to each data service, in the case of subgraphs this is an ABI-encoded SubgraphIndexingVoucherMetadata
#[derive(Debug, RlpEncodable, RlpDecodable, PartialEq)]
struct SubgraphIndexingVoucherMetadata {
uint256 pricePerBlock; // wei GRT
bytes32 protocolNetwork; // eip199:1 format
// differentiate based on indexed chain
bytes32 chainId; // eip199:1 format
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for the future: We might want this to be part of the thegraph_core crate to share the same code between the different parts.

dips/src/lib.rs Outdated

let payer = sig
.recover_address_from_msg(self.voucher.eip712_hash_struct())
.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to return a false instead of panicking.

EDIT: +1 to the error type on the TODO comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

EIP-712 domain separator is an essential part of the signing schema. I miss it here.

Check this implementation for an example: https://github.com/edgeandnode/toolshed/blob/e7ea9a8209b7a960b4c3bd4e9a0dd2f2ea9019a2/thegraph-core/src/attestation.rs#L70-L79


#[async_trait]
pub trait AgreementStore: Sync + Send {
async fn get_by_signature(&self, signature: String) -> anyhow::Result<Option<Agreement>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am considering using a UUIDv7 as the agreement identifier instead of the signature. It has some good properties that we can use to manage and trace data across the different DIPs' parties (i.e., DIPs Gateway/Dipper, Indexer, and future smart contracts).

Of course, these IDs will be only relevant on the indexer-facing side of the DIPs domain.

WDYT?

Copy link
Contributor

@LNSD LNSD Oct 8, 2024

Choose a reason for hiding this comment

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

For example, as UUIDv7 is lexicographically sortable, we can quickly sort the agreements from oldest to newest without requiring a timestamp field.

Additionally, I feel more comfortable providing a UUID as a "path parameter" in a RESTful API than a base64 RLP encoded signature because we would need to use the "URL-friendly" flavor of the Base64 encoding.

@mangas mangas force-pushed the dips-graphql branch 5 times, most recently from 2c49fc1 to bd31868 Compare October 14, 2024 09:31
@mangas mangas marked this pull request as ready for review October 14, 2024 09:32
@mangas mangas marked this pull request as draft October 14, 2024 09:33
@mangas mangas force-pushed the dips-graphql branch 4 times, most recently from 641ad4d to 2fbf0c3 Compare October 14, 2024 10:02
@mangas mangas marked this pull request as ready for review October 14, 2024 11:46
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.

3 participants