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 partial account filters for near #3669

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Conversation

mangas
Copy link
Contributor

@mangas mangas commented Jun 15, 2022

Fixes #3628

@mangas mangas force-pushed the near-partial-account-filters branch from ae831df to 701370d Compare June 15, 2022 22:00
@mangas mangas requested a review from leoyvens June 15, 2022 22:02
@mangas mangas force-pushed the near-partial-account-filters branch from 701370d to dce8459 Compare June 17, 2022 14:10
@mangas mangas requested a review from evaporei June 17, 2022 15:44
Comment on lines 145 to 146
// prefix [] and suffiex [c, d] would produce [None, c], [None, d]
// prefix [a,b] and suffiex [] would produce [a, None], [b, None]
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here on suffix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +11 to +15
// PrefixSuffixPair applies a logical AND to prefix and suffix when both fields are non-empty.
// * {prefix="hello",suffix="world"} will match "hello.world" but not "hello.friend"
// * {prefix="hello",suffix=""} will match both "hello.world" and "hello.friend"
// * {prefix="",suffix="world"} will match both "hello.world" and "good.day.world"
// * {prefix="",suffix=""} is invalid
Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice to have docs here, thanks 👍

Comment on lines 465 to +468
pub(crate) account: Option<String>,
#[serde(rename = "startBlock", default)]
pub(crate) start_block: BlockNumber,
pub(crate) accounts: Option<PartialAccounts>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that account and accounts are optional. What happens if one tries to deploy a NEAR subgraph that has both values as None? Do you know if we have validation for this or if is it valid?

if no_source_address && has_receipt_handlers {

// Validate not both address and partial addresses are empty.
if (no_source_address && no_partial_addresses) && has_receipt_handlers {
errors.push(SubgraphManifestValidationError::SourceAddressRequired.into());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind about my other None comment, the validation is here 🙂

.map(|s| s.account.as_ref().cloned().unwrap())
.collect();

// Parse all the partial accounts, produces all possible combiantions of the values
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in combinations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@mangas mangas force-pushed the near-partial-account-filters branch from a03c891 to 7a3aa52 Compare June 20, 2022 15:51
@mangas mangas merged commit 28580bb into master Jun 21, 2022
@mangas mangas deleted the near-partial-account-filters branch June 21, 2022 22: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.

Dynamic data sources for NEAR accounts
2 participants