drop query proofs in casting and agoric-cli #11022
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
7540e4e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e82e30a4.agoric-sdk.pages.dev |
| Branch Preview URL: | https://ta-drop-query-proofs.agoric-sdk.pages.dev |
| return E(queryClient).queryStoreVerified(storeName, storeSubkey, height); | ||
| }); | ||
| console.error('getProvenDataAtHeight', height, 'is no longer supported'); | ||
| throw makeError(X`Verified queries are no longer supported`); |
There was a problem hiding this comment.
This doesn't break anything? I'd expect at least that the old default "optimistic" starts showing a lot of errors/warnings, but I'm not sure where, and tests won't exactly catch noisy logs. Maybe we could make a temporary change to fail earlier if the proof is "optimistic" so we can see if any tests fail? Then we'd know which code is impacted.
There was a problem hiding this comment.
It's marked as a breaking change, but no CI tests were affected. I don't think anyone was opting into optimistic or strict, but if they were there's not really anything more we can do. There's no implementation anymore.
In this case I think it's best to fail loudly so any code expecting verification fails and can be updated.
There was a problem hiding this comment.
In this case I think it's best to fail loudly so any code expecting verification fails and can be updated.
Right, this will fail loudly for strict, but for optimistic which is the default, the follower will yield the data and then throw later when this proof verification fails. I don't think this would "break" anything, but result in some noisy logs saying the proofs failed. I was thinking it might be better to throw earlier, so that we can catch those cases, but if no one was using optimistic then that's moot.
There was a problem hiding this comment.
That would be a problem but I'm pretty sure this PR changes the default to 'none'.
| return value; | ||
| }, | ||
| 'optimistic', | ||
| ) | ||
| .option( |
There was a problem hiding this comment.
Please don't make an unnecessary breaking change here. --proof=none is still legitimate.
| .option( | |
| .option( | |
| '--proof <none>', | |
| `set proof mode (currently only 'none' is supported)`, | |
| value => { | |
| assert.equal( | |
| value, | |
| 'none', | |
| X`--proof can only be 'none'`, | |
| TypeError, | |
| ); | |
| return value; | |
| }, | |
| 'none', | |
| ) | |
| .option( |
There was a problem hiding this comment.
fair. I'll include something about the option being deprecated since it doesn't do anything.
There was a problem hiding this comment.
I edited the above to suggest something better than before. Please take it into account.
| console.error('getProvenDataAtHeight', height, 'is no longer supported'); | ||
| throw makeError(X`Verified queries are no longer supported`); |
There was a problem hiding this comment.
| console.error('getProvenDataAtHeight', height, 'is no longer supported'); | |
| throw makeError(X`Verified queries are no longer supported`); | |
| console.error('getProvenDataAtHeight', height, 'is no longer supported; use', { proof: 'none' }); | |
| throw makeError(X`Verified queries are no longer supported; use { proof: 'none' }`); |
incidental
Description
CosmJS dropped support for query proofs because the JS implementation was removed from ics23.
There is no supported implementation so this removes the feature.
It leaves the code structure intact in case we can find another implementation, so the earlier API can be restored.
It drops the option entirely from agoric-cli so users aren't shown it in help and autocomplete.
Security Considerations
Loss of client-side proofs.
Scaling Considerations
none
Documentation Considerations
Code comments
Testing Considerations
CI
Upgrade Considerations
I can't find anything that was using it.