fix Telescope encoding Decimal #10080
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dckc
approved these changes
Sep 12, 2024
| @@ -74,3 +75,21 @@ test('build Query Packet', t => { | |||
| 'eyJkYXRhIjoiQ2pvS0ZBb0xZMjl6Ylc5ek1YUmxjM1FTQlhWaGRHOXRFaUl2WTI5emJXOXpMbUpoYm1zdWRqRmlaWFJoTVM1UmRXVnllUzlDWVd4aGJtTmwiLCJtZW1vIjoiIn0=', | |||
| ); | |||
| }); | |||
|
|
|||
| test('build QueryDelegatorDelegationsResponse', t => { | |||
There was a problem hiding this comment.
I think I ran into trouble parsing before. Do you know whether we already have a test for that? It seems pretty cheap to add one here.
| @@ -136,7 +144,7 @@ | |||
| "devDependencies": { | |||
| "@agoric/cosmos": "^0.34.1", | |||
| "@ava/typescript": "^4.1.0", | |||
| "@cosmology/telescope": "^1.7.1", | |||
| "@cosmology/telescope": "https://gitpkg.vercel.app/agoric-labs/telescope/packages/telescope?8d2c2f6ba637a5578eead09a7368dc41c262a9d0", | |||
There was a problem hiding this comment.
Confirm my understanding here, please? The reason for gitpkg rather than a link to github.com is that the package we want is not at the top level?
There was a problem hiding this comment.
Ah... I see internal discussion Aug 20 to confirm that others in the shop know about gitpkg.
Deploying agoric-sdk with
|
| Latest commit: |
c1c5a6f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://036f7b54.agoric-sdk.pages.dev |
| Branch Preview URL: | https://10072-bigint-decimal.agoric-sdk.pages.dev |
0xpatrickdev
approved these changes
Sep 13, 2024
| */ | ||
| export const TimestampProtoShape = { seconds: M.nat(), nanos: M.number() }; | ||
| export const TimestampProtoShape = { seconds: M.string(), nanos: M.number() }; |
There was a problem hiding this comment.
fwiw I didn't catch this analytically. it was caught by your improvement to JsonSafe in telescope that we finally have in agoric-sdk.
That required this change:
return /** @type {JsonSafe<MsgUndelegateResponse>} */ ({
// 5 seconds from unix epoch
- completionTime: { seconds: 5n, nanos: 0 },
+ completionTime: { seconds: '5', nanos: 0 },Which then required the guard change.
turadg
force-pushed
the
10072-bigint-Decimal
branch
from
22ca1fc
to
136f1ca
Compare
turadg
force-pushed
the
10072-bigint-Decimal
branch
from
136f1ca
to
c1c5a6f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #10072
Description
We can't use cosmjs/math because it uses bn.js which is huge. They've decided not to remove it from cosmjs/math so we patched Telescope to use its own Decimal module. However the subset chosen started breaking with the use of Query modules.
Updating the patch was a giant pain so instead this sources from a fork, https://github.com/agoric-labs/telescope/tree/agoric-safe/
When updating that fork, the assets have to be build and pushed because they're not published to NPM.
Also, the code built should be compatible with what's currently published to NPM because the other packages will be sourced from there. Note that this fork is currently behind upstream (agoric-labs/telescope@agoric-safe...cosmology-tech:telescope:main ) because this commit requires an unpublished @cosmology/utils (symption is
toPosixPathundefined).Security Considerations
sources from a repo using gitpkg.vercel.app which could modify the contents in transit. The yarn.lock doesn't have an integrity check for Git sources. However this code is only used for codegen so any malice or even error would be detected in the new output.
Scaling Considerations
n/a
Documentation Considerations
The fork updating process is not obvious. I hope the PR description suffices.
Testing Considerations
The new
build QueryDelegatorDelegationsResponsetest fails under master, showing that it solves the problem.Upgrade Considerations
n/a