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 support and tests for capability specification via provides #80

Merged
merged 3 commits into from
Feb 5, 2025

Conversation

pipex
Copy link
Contributor

@pipex pipex commented Dec 30, 2024

This updates the satisfiesChildContract and getNotSatisfiedRequirements functions to check for capabilities defined via provides on the current context. This PR is just surfacing some functionality that was already partially there but never tested.

Change-type: minor
Depends-on: #79

@pipex pipex marked this pull request as draft December 30, 2024 14:05
@pipex pipex force-pushed the capability-support branch 10 times, most recently from eeb6ea9 to 1c1c1a3 Compare January 7, 2025 21:40
@pipex pipex force-pushed the capability-support branch 4 times, most recently from 2180cbb to 978c919 Compare January 15, 2025 16:34
@pipex pipex marked this pull request as ready for review January 15, 2025 16:34
@pipex pipex requested a review from a team January 15, 2025 16:34
@flowzone-app flowzone-app bot enabled auto-merge January 15, 2025 16:37
@pipex pipex force-pushed the capability-support branch from 286e75f to f6b5485 Compare February 3, 2025 18:38
lib/index.ts Outdated Show resolved Hide resolved
lib/contract.ts Outdated Show resolved Hide resolved
lib/contract.ts Outdated Show resolved Hide resolved
lib/contract.ts Outdated Show resolved Hide resolved
Comment on lines +1077 to +1088
const disjuncts = filter(requirement.raw.data.getAll(), (disjunct) => {
return shouldEvaluateType(disjunct.raw.data.type);
});
// (3.2) An empty disjuction means that this particular
// requirement is fulfilled, so we can carry on.
// A disjunction naturally contains a list of further
// requirements we need to check for. If at least one
// of the members is fulfilled, we can proceed with
// next requirement.
if (disjuncts.length === 0 || some(disjuncts, hasMatch)) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm less confident on this combination but it should work (I may have done the conversion incorrectly, but the conversion into a single every should be possible)

Suggested change
const disjuncts = filter(requirement.raw.data.getAll(), (disjunct) => {
return shouldEvaluateType(disjunct.raw.data.type);
});
// (3.2) An empty disjuction means that this particular
// requirement is fulfilled, so we can carry on.
// A disjunction naturally contains a list of further
// requirements we need to check for. If at least one
// of the members is fulfilled, we can proceed with
// next requirement.
if (disjuncts.length === 0 || some(disjuncts, hasMatch)) {
return true;
}
// (3.2) An empty disjuction means that this particular
// requirement is fulfilled, so we can carry on.
// A disjunction naturally contains a list of further
// requirements we need to check for. If at least one
// of the members is fulfilled, we can proceed with
// next requirement.
if (every(requirement.raw.data.getAll(), (disjunct) => {
return !shouldEvaluateType(disjunct.raw.data.type) || !hasMatch(disjunct)
})) {
return true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work unfortunately, in order for the requirement to be accepted we need either:

  • there are no requirements to evaluate
  • if there are requirements to evaluate, there is at least one match

The closest thing would be

if (
	every(requirement.raw.data.getAll(), (disjunct) => {
		return (
			!shouldEvaluateType(disjunct.raw.data.type) || hasMatch(disjunct)
		);
	})
) {
	return true;
}

however this is not the same, because if shouldEvaluateType(disjunct.raw.data.type) is always true, this is equivalent to writing

if (
	every(requirement.raw.data.getAll(), (disjunct) => {
		return hasMatch(disjunct);
	})
) {
	return true;
}

which is the opposite of what we want

I'm not sure there is an equivalent using a single some/every

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the problem is we need every(disjuncts, !shouldEvaluateType) || some(disjuncts, shouldEvaluateType && hasMatch) and those can't be combined so what we currently have is the best we can manage with functional. However it can be done via a for-loop with return to short-circuit the some case but keeping the every case requiring an entire loop:

Suggested change
const disjuncts = filter(requirement.raw.data.getAll(), (disjunct) => {
return shouldEvaluateType(disjunct.raw.data.type);
});
// (3.2) An empty disjuction means that this particular
// requirement is fulfilled, so we can carry on.
// A disjunction naturally contains a list of further
// requirements we need to check for. If at least one
// of the members is fulfilled, we can proceed with
// next requirement.
if (disjuncts.length === 0 || some(disjuncts, hasMatch)) {
return true;
}
// (3.2) An empty disjuction means that this particular
// requirement is fulfilled, so we can carry on.
// A disjunction naturally contains a list of further
// requirements we need to check for. If at least one
// of the members is fulfilled, we can proceed with
// next requirement.
let noneToEvaluate = true;
for (const disjunct of (requirement.raw.data.getAll()) {
if (shouldEvaluateType(disjunct.raw.data.type)) {
noneToEvaluate = false
if (hasMatch(disjunct)) {
return true;
}
}
}
return noneToEvaluate;

Fwiw I don't think this is worth it unless it's a performance bottleneck but since I dug into how to do it I thought I'd share

Copy link
Contributor Author

@pipex pipex Feb 5, 2025

Choose a reason for hiding this comment

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

It does sacrifice a bit of readability, I suspect at some point we'll need to work on the efficiency of some of the operations at which point we can review. Thank you for looking into it!

@pipex pipex force-pushed the capability-support branch 2 times, most recently from 5198c53 to 63db5d0 Compare February 4, 2025 22:23
lib/contract.ts Outdated Show resolved Hide resolved
pipex added 3 commits February 4, 2025 23:24
This removes some code duplication between `satisfiesChildContract` and
`getNotSatisfiedRequirements`

Change-type: patch
This lines up better with the original specification and how contracts
are being used by the OS

Change-type: minor
Using array cardinalities would result in the blueprint using the
`Array.filter` function as the filter for the selector, causing the
filtered set to always have size 0

Change-type: patch
@pipex pipex force-pushed the capability-support branch from 63db5d0 to a79b696 Compare February 5, 2025 02:24
@flowzone-app flowzone-app bot merged commit 6524517 into master Feb 5, 2025
50 checks passed
@flowzone-app flowzone-app bot deleted the capability-support branch February 5, 2025 10:57
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.

2 participants