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: predicate validation #611

Merged
merged 40 commits into from
Jul 5, 2024
Merged

feat: predicate validation #611

merged 40 commits into from
Jul 5, 2024

Conversation

MicaiahReid
Copy link
Contributor

@MicaiahReid MicaiahReid commented Jul 1, 2024

Description

This PR adds validation for predicates. The ChainhookSpecificationNetworkMap::validate(&self) method will now check each of the fields in a predicate that could have invalid data and return a string with all of the errors separated by a "\n".

I'm open to other ways of formatting the returned errors, but I think it will be nice for users to see everything that is wrong with their spec in the first use rather than being given just the first error.

Example

Here is an example result:

invalid Stacks predicate 'predicate_name' for network simnet: invalid 'then_that' value: invalid 'http_post' data: url string must be a valid Url: relative URL without a base
invalid Stacks predicate 'predicate_name' for network simnet: invalid 'then_that' value: invalid 'http_post' data: auth header must be a valid header value: failed to parse header value
invalid Stacks predicate 'predicate_name' for network simnet: invalid 'if_this' value: invalid predicate for scope 'print_event': invalid contract identifier: ParseError("Invalid principal literal: base58ck checksum 0x147e6835 does not match expected 0x9b3dfe6a")

Checklist

  • All tests pass
  • Tests added in this PR (if applicable)

removed - `cache_path`, `data_handler_tx`, and`ingestion_port`
renamed - `display_logs` -> `display_stacks_ingestion_logs`
`*ChainhookFullSpecification` => `*ChainhookSpecificationNetworkMap`
`*ChainhookNetworkSpecification` => `*ChainhookSpecification`
`*ChainhookSpecification` => `*ChainhookInstance`
remove unnecessary wrapper around ChainhookStore
@MicaiahReid MicaiahReid marked this pull request as ready for review July 1, 2024 20:59
@MicaiahReid MicaiahReid temporarily deployed to Development-mainnet July 1, 2024 21:35 — with GitHub Actions Inactive
@MicaiahReid MicaiahReid temporarily deployed to Development-testnet July 1, 2024 21:35 — with GitHub Actions Inactive
@tippenein
Copy link
Contributor

Looks to me like this fixes #610

@MicaiahReid
Copy link
Contributor Author

@tippenein Not quite.

The validation function for the regex does generate the regex, but we aren't actually mutating the original struct. We'd have to add another field to store the computed regex, and we'd also have to update this validate function to mutate the predicate to store this struct.

I think where this gets tricky is that we also have to propagate this change from the ChainhookSpecification struct (where the validation happens) to the ChainhookSpecificationNetworkMap struct. When running a service, the POST /api/v1/chainhooks route takes a ChainhookSpecificationNetworkMap, validates each of its ChainhookSpecifications, then registers the network map. So for these cached values to actually get propagated through to where we're storing the predicates, we'd need to be sure everything is properly mutated.

This approach feels a bit messy to me, especially considering we'd be doing these mutations on a function called validate. I wouldn't call a validate function and assume it to have side effects like mutating the data we're validating.


Maybe for #610 we can add a function like pre_compute_predicate that's called closer to where we actually register the predicates. We could compute regex, the wallet descriptors on the bitcoin side, and there will likely be future predicates that could benefit from a pre computation stage.

@MicaiahReid MicaiahReid merged commit 67e28b1 into main Jul 5, 2024
13 of 15 checks passed
@MicaiahReid MicaiahReid deleted the feat/predicate-validation branch July 5, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants