-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
drop query proofs in casting and agoric-cli #11022
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -92,6 +92,10 @@ const collectSingle = values => { | |||||||||
return head[0]; | ||||||||||
}; | ||||||||||
|
||||||||||
// NB: 'none' is the only value that works. We've left the other cases | ||||||||||
// in anticipation of the ecosystem providing JS proofs again. | ||||||||||
// See https://github.com/cosmos/cosmjs/issues/1618#issuecomment-2574934505 | ||||||||||
// and https://github.com/cosmos/ics23/pull/353?email_source=slack | ||||||||||
// Coordinate with switch/case of tryGetDataAtHeight. | ||||||||||
const proofs = ['strict', 'none', 'optimistic']; | ||||||||||
|
||||||||||
|
@@ -110,7 +114,7 @@ export const makeCosmjsFollower = ( | |||||||||
const { | ||||||||||
decode = MAKE_DEFAULT_DECODER(), | ||||||||||
unserializer = MAKE_DEFAULT_UNSERIALIZER(), | ||||||||||
proof = 'optimistic', | ||||||||||
proof = 'none', | ||||||||||
crasher = null, | ||||||||||
} = options; | ||||||||||
|
||||||||||
|
@@ -217,14 +221,13 @@ export const makeCosmjsFollower = ( | |||||||||
}; | ||||||||||
|
||||||||||
/** | ||||||||||
* @deprecated no longer supported https://github.com/cosmos/cosmjs/pull/1623 | ||||||||||
* @param {number} [height] | ||||||||||
* @returns {Promise<QueryStoreResponse>} | ||||||||||
*/ | ||||||||||
const getProvenDataAtHeight = async height => { | ||||||||||
return retryGetPrefixedData(async (endpoint, storeName, storeSubkey) => { | ||||||||||
const queryClient = await provideQueryClient(endpoint); | ||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, this will fail loudly for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be a problem but I'm pretty sure this PR changes the default to 'none'.
Comment on lines
+229
to
+230
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
}; | ||||||||||
|
||||||||||
/** | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't make an unnecessary breaking change here.
--proof=none
is still legitimate.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair. I'll include something about the option being deprecated since it doesn't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I edited the above to suggest something better than before. Please take it into account.