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

proto messages in vats #9112

Merged
merged 16 commits into from
Apr 8, 2024
Merged

proto messages in vats #9112

merged 16 commits into from
Apr 8, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Mar 19, 2024

refs: #9111

Description

#9111 specified to support both encodings in VLOCAL_CHAIN_EXECUTE_TX. Instead, @michaelfig determined it would be better to have the one encoding of Protobuf 3 Any into JSON, and have userland utilities to convert any encoding into that. We considered supporting a binary encoding but those can't go over the bridge without hacks like the lower 8 bits of each character in a string.

To support the proto messages in vats, I had to kill imports that were assuming a browser/Node environment (e.g. axios that expected http and stream). We wanted to use those for off-chain clients but it's not necessary for Orchestration. It will take more work at some point: #9200

Security Considerations

No change.

Scaling Considerations

Larger bundles when the registry is imported.

Documentation Considerations

We'll want to document these utilities as part of the Orchestration API.

Testing Considerations

CI

Upgrade Considerations

The only runtime change to a production vat is a validation that the list given to executeTx isn't empty. Even that isn't an upgrade consideration because vat-localchain isn't deployed yet.

@turadg turadg force-pushed the 9111-proto-messages branch 7 times, most recently from 84f66f1 to 07c85b3 Compare March 26, 2024 22:12
@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Mar 26, 2024
Copy link

@turadg - This PR appears to be stuck. It's had a merge label for > 24 hours

2 similar comments
Copy link

@turadg - This PR appears to be stuck. It's had a merge label for > 24 hours

Copy link

github-actions bot commented Apr 1, 2024

@turadg - This PR appears to be stuck. It's had a merge label for > 24 hours

@turadg turadg force-pushed the 9111-proto-messages branch from 07c85b3 to d574e9b Compare April 1, 2024 18:25
@turadg turadg mentioned this pull request Apr 1, 2024
@turadg turadg changed the base branch from master to 9111-cosmos-bufs April 1, 2024 21:46
@turadg turadg force-pushed the 9111-proto-messages branch from d574e9b to 3744406 Compare April 1, 2024 21:46
@turadg turadg removed the automerge:rebase Automatically rebase updates, then merge label Apr 1, 2024
@turadg turadg force-pushed the 9111-proto-messages branch from 3744406 to 5d76df1 Compare April 1, 2024 22:44
@turadg turadg force-pushed the 9111-cosmos-bufs branch 2 times, most recently from 135b911 to c5769b4 Compare April 4, 2024 17:38
Base automatically changed from 9111-cosmos-bufs to master April 4, 2024 18:10
@turadg turadg force-pushed the 9111-proto-messages branch 4 times, most recently from 844de25 to 32f2624 Compare April 5, 2024 15:48
@turadg turadg marked this pull request as ready for review April 5, 2024 15:51
@turadg turadg changed the title 9111 proto messages proto messages in vats Apr 5, 2024
@turadg turadg force-pushed the 9111-proto-messages branch from 32f2624 to 25790ee Compare April 5, 2024 16:05
@turadg turadg mentioned this pull request Apr 5, 2024
packages/casting/test/test-interpose-net-access.js Outdated Show resolved Hide resolved
@@ -86,13 +86,13 @@ telescope({
},
aminoEncoding: {
// Necessary for getSigningAgoricClient
enabled: true,
enabled: false,
Copy link
Member

Choose a reason for hiding this comment

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

dapp-inter and other web clients are likely dependent on amino encodings. Agree they should be removed for vats, but something to make sure is in scope as part of #9200

Copy link
Member Author

Choose a reason for hiding this comment

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

Good callout, though it won't break until they try to upgrade past 0.4.0 and by then we may have #9200. If not then they shouldn't upgrade. I expect their CIs to fail if they upgrade to a version lacking the requisite functionality.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Apr 5, 2024
@turadg turadg force-pushed the 9111-proto-messages branch from 7fb4ebc to 2a0fa0a Compare April 6, 2024 15:41
@turadg turadg force-pushed the 9111-proto-messages branch from bc4283b to bc464b0 Compare April 8, 2024 00:45
@turadg turadg force-pushed the 9111-proto-messages branch from 6482df5 to 722015f Compare April 8, 2024 14:29
t.deepEqual(qabr, {
'@type': '/cosmos.bank.v1beta1.QueryAllBalancesRequest',
address,
other: 3, // retained because there's no runtime validation
Copy link
Member

Choose a reason for hiding this comment

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

nb: we may want to consider using .fromPartial() to trim extra fields. It seems this is available via registry

:latest was bumped to include u14 by Agoric/agoric-3-proposals#141

but master can't yet apply on top of u14 #9204
@mergify mergify bot merged commit c74c628 into master Apr 8, 2024
66 checks passed
@mergify mergify bot deleted the 9111-proto-messages branch April 8, 2024 16:10
mergify bot pushed a commit that referenced this pull request Jun 22, 2024
closes: #XXXX
refs: endojs/endo#1837 7accc02 #9112 https://github.com/endojs/endo/blob/master/packages/ses/error-codes/SES_HTML_COMMENT_REJECTED.md

## Description

A patch introduced in at 7accc02 in #9112 patched https://www.npmjs.com/package/bn.js/v/5.1.2 to work around the bug explained at endojs/endo#1837 . However, the fix followed the advice at endojs/endo#1837 (comment) , which is wrong for the reasons explained at endojs/endo#1837 (comment) .
- wrong: rewrite `x-- > y` as `(x--, x > y)`

This PR fixes that mistake by instead using the technique @gibson042 suggests at endojs/endo#1837 (comment)
- correct: rewrite `x-- > y` as `[x--][0] > y`

### Security Considerations
fixes an integrity bug. I have no idea how significant this bug was.
### Scaling Considerations
none
### Documentation Considerations
none
### Testing Considerations
none
### Upgrade Considerations
Well, it is a change. But I have no idea what the patched library was used for, so cannot evaluate.
mhofman pushed a commit that referenced this pull request Jun 22, 2024
closes: #XXXX
refs: endojs/endo#1837 7accc02 #9112 https://github.com/endojs/endo/blob/master/packages/ses/error-codes/SES_HTML_COMMENT_REJECTED.md

## Description

A patch introduced in at 7accc02 in #9112 patched https://www.npmjs.com/package/bn.js/v/5.1.2 to work around the bug explained at endojs/endo#1837 . However, the fix followed the advice at endojs/endo#1837 (comment) , which is wrong for the reasons explained at endojs/endo#1837 (comment) .
- wrong: rewrite `x-- > y` as `(x--, x > y)`

This PR fixes that mistake by instead using the technique @gibson042 suggests at endojs/endo#1837 (comment)
- correct: rewrite `x-- > y` as `[x--][0] > y`

### Security Considerations
fixes an integrity bug. I have no idea how significant this bug was.
### Scaling Considerations
none
### Documentation Considerations
none
### Testing Considerations
none
### Upgrade Considerations
Well, it is a change. But I have no idea what the patched library was used for, so cannot evaluate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants