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

build cosmic-proto with Telescope #8931

Merged
merged 28 commits into from
Feb 29, 2024
Merged

build cosmic-proto with Telescope #8931

merged 28 commits into from
Feb 29, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Feb 15, 2024

closes: #8917

Description

Will change @agoric/cosmic-proto to build with Telescope, giving us better helper code.

I tried to keep the commits clean. The package started generated asnew-cosmic-proto and I replace the existing one after nailing some thing down.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

green PR in dapp-inter:

green PR in wallet-app

  • TODO

green PR in ui-kit :

green PR in cosmos-proposal-builder

agoric-3-proposals had some references, but they were dead code,

Upgrade Considerations

@turadg turadg requested review from dckc and michaelfig February 15, 2024 23:44
@dckc
Copy link
Member

dckc commented Feb 16, 2024

test plan looks as good as anything I can think of.
that same list is what you mean by requirements, yes?

IIUC, it should address...

Here's hoping for a moment to try it.

I'm also interested to see how it goes with test-interpose-net-access.js in casting.

@dckc
Copy link
Member

dckc commented Feb 16, 2024

140KLOC. oh my.
Separating the generated code from the stuff we're maintaining by hand will be important for review.

@turadg
Copy link
Member Author

turadg commented Feb 16, 2024

that same list is what you mean by requirements, yes?

I suppose so.

Separating the generated code from the stuff we're maintaining by hand will be important for review.

Exactly the code in src/codegen is generated.

@turadg turadg force-pushed the 8917-telescope branch 7 times, most recently from 863515d to af55111 Compare February 21, 2024 01:33
package.json Outdated Show resolved Hide resolved
@turadg turadg force-pushed the 8917-telescope branch 3 times, most recently from a19dee0 to f67bda7 Compare February 26, 2024 20:14
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

This is a good start! I would like to see some effort towards reducing the amount of noise introduced by Telescope, such as support for CommonJS and the semantically void codegen/ and dist/ directory names. But by all means, full steam ahead!

packages/agoric-cli/src/commands/wallet.js Outdated Show resolved Hide resolved
@@ -26,6 +26,7 @@ import {
import type { ExecutionContext as AvaT } from 'ava';

import { makeRunUtils } from '@agoric/swingset-vat/tools/run-utils.js';
import { CoreEvalSDKType } from '@agoric/cosmic-proto/dist/codegen/agoric/swingset/swingset';
Copy link
Member

@michaelfig michaelfig Feb 27, 2024

Choose a reason for hiding this comment

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

Suggested change
import { CoreEvalSDKType } from '@agoric/cosmic-proto/dist/codegen/agoric/swingset/swingset';
import type { CoreEvalSDKType } from '@agoric/cosmic-proto/agoric/swingset/swingset';

Copy link
Member Author

@turadg turadg Feb 27, 2024

Choose a reason for hiding this comment

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

I've added type and exports for swingset/swingset.js.

Importing it here would be nice. Unfortunately it will only work with NodeNext moduleResolution and enabling that breaks our use of ambient types (3364 errors).

Punting to #9005

EDIT: I see your other comment about moving dist/codegen/* to the package level. I'll discuss that there.

packages/cosmic-proto/.gitignore Outdated Show resolved Hide resolved
packages/cosmic-proto/README.md Outdated Show resolved Hide resolved
packages/cosmic-proto/README.md Outdated Show resolved Hide resolved
packages/cosmic-proto/package.json Outdated Show resolved Hide resolved
const rimraf = require('rimraf').rimrafSync;

const protoDirs = [join(__dirname, '/../proto')];
const outPath = join(__dirname, '../src/codegen');
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer this, since "codegen" is just noise.

Suggested change
const outPath = join(__dirname, '../src/codegen');
const outPath = join(__dirname, '../src');

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't it helpful to distinguish which source files were generated by machine vs by human? @dckc asked for this explicitly in review

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is the only hand-written source file in the src directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. And you can tell that because it's not in the codegen directory

},
rpcClients: {
enabled: true,
camelCase: true,
Copy link
Member

Choose a reason for hiding this comment

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

Is this the setting that required changes in agoric-cli for the returned swingset params?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose so. Are you opposed to it? To me the camelCase is what one would expect in a JS API.

Copy link
Member Author

Choose a reason for hiding this comment

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

@samsiegart you've written the most client code. What's your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd guess camelCase, but not sure where this shows up

Copy link
Member Author

Choose a reason for hiding this comment

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

@michaelfig made a good point about sticking to defaults. I took this option out to adopt the defaults. Turns out true is the Telescope default

Copy link
Member Author

Choose a reason for hiding this comment

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

@michaelfig made a good point about sticking to defaults. I took this option out to adopt the defaults. Turns out true is the Telescope default.

@samsiegart it shows up in 7c01cb0

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yea, camelCase please 👍

Copy link
Member Author

@turadg turadg Feb 29, 2024

Choose a reason for hiding this comment

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

I found the option that determines power_flag_fees vs powerFlagFees: parser.keepCase.

Setting it to keepCase: true matches the older snake_case. Removing it uses the camelCase, so that is the default of Telescope. Given our desire to match the defaults I've left it alone. I considered removing it but I think it's helpful for discovery.

packages/agoric-cli/src/commands/wallet.js Show resolved Hide resolved
@turadg turadg force-pushed the 8917-telescope branch 2 times, most recently from e954f2c to 3b88b7b Compare February 28, 2024 01:21
@turadg turadg marked this pull request as ready for review February 28, 2024 14:46
@turadg turadg requested a review from michaelfig February 28, 2024 14:46
@turadg turadg marked this pull request as draft February 28, 2024 18:18
@turadg turadg force-pushed the 8917-telescope branch 2 times, most recently from 855f30b to f71b722 Compare February 28, 2024 22:11
@turadg turadg marked this pull request as ready for review February 28, 2024 22:11
@turadg turadg requested a review from 0xpatrickdev February 28, 2024 22:11
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

No need to adopt any of these suggestions right now... I'm fine with putting them in a future PR.


### Codegen

Contract schemas live in `./contracts`, and protos in `./proto`. Look inside of `scripts/codegen.cjs` and configure the settings for bundling your SDK and contracts into `@agoric/cosmic-proto`:
Copy link
Member

Choose a reason for hiding this comment

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

We don't use "Contract schemas" or "bundling contracts into @agoric/cosmic-proto":

Suggested change
Contract schemas live in `./contracts`, and protos in `./proto`. Look inside of `scripts/codegen.cjs` and configure the settings for bundling your SDK and contracts into `@agoric/cosmic-proto`:
Protos live in `./proto`. Look inside of `scripts/codegen.cjs` and configure the settings for bundling your SDK into `@agoric/cosmic-proto`:

- [Usage](#usage)
- [RPC Clients](#rpc-clients)
- [Composing Messages](#composing-messages)
- Cosmos, CosmWasm, and IBC
Copy link
Member

Choose a reason for hiding this comment

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

For future reference, please search the readme for all instances of (case-insensitive) "cosmwasm" and remove them as they may confuse somebody writing an Agoric smart contract.

Comment on lines +9 to +15
+ const importStmts = bundler.bundle.importPaths;
+ for (const stmt of importStmts) {
+ if (stmt.source.value.startsWith('.') && !stmt.source.value.endsWith('.js')) {
+ stmt.source.value += '.js';
+ }
+ }
+
Copy link
Member

Choose a reason for hiding this comment

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

Nicely done!

Comment on lines +32 to +41
- const reducedDescriptors = {};
+ let reducedDescriptors = {};

forEach(descriptors, (descriptor, name) => {
let ret;
if ((ret = reducer(descriptor, name, obj)) !== false) {
- reducedDescriptors[name] = ret || descriptor;
+ reducedDescriptors = {...reducedDescriptors,
+ [name]: ret || descriptor
+ };
Copy link
Member

Choose a reason for hiding this comment

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

FFR, we should probably use Object.defineProperty(reducedDescriptors, name, { value: ret || descriptor }) since otherwise this loop is quadratic.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I did first but when PRing upstream it caused a test failure.

@mergify mergify bot merged commit 072c457 into master Feb 29, 2024
66 checks passed
@mergify mergify bot deleted the 8917-telescope branch February 29, 2024 23:24
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.

adopt Telescope
5 participants