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

use local codegen for protobuf message types #1624

Merged
merged 16 commits into from
Aug 13, 2024
Merged

Conversation

turbocrime
Copy link
Contributor

fixes #1483 #1503

replaces #1582 #1583

it's simplest to use buf generate to create the definitions

Copy link

changeset-bot bot commented Aug 2, 2024

🦋 Changeset detected

Latest commit: bf60c3f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
@penumbra-zone/protobuf Major
minifront Patch
node-status Patch
@penumbra-zone/bech32m Major
@penumbra-zone/client Major
@penumbra-zone/getters Major
@penumbra-zone/perspective Major
@penumbra-zone/query Major
@penumbra-zone/services Major
@penumbra-zone/storage Major
@penumbra-zone/types Major
@repo/ui Major
@penumbra-zone/wasm Major
@penumbra-zone/crypto-web Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

"@buf/tendermint_tendermint.bufbuild_es": "1.10.0-20231117195010-33ed361a9051.1",
"@bufbuild/protobuf": "^1.10.0",
"@connectrpc/connect": "^1.4.0",
"@connectrpc/connect-web": "^1.4.0"
},
"devDependencies": {
"@buf/penumbra-zone_penumbra.bufbuild_es": "1.10.0-20240703080008-312294d02bf9.1",
Copy link
Contributor Author

@turbocrime turbocrime Aug 2, 2024

Choose a reason for hiding this comment

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

this one buf dep is still necessary because the registry incorrectly fails to declare a dependency

Comment on lines 14 to 18
"gen:cosmos-sdk": "buf generate buf.build/cosmos/cosmos-sdk:e7a85cef453e4b999ad9aff8714ae05f",
"gen:ibc": "buf generate buf.build/cosmos/ibc:7ab44ae956a0488ea04e04511efa5f70",
"gen:ics23": "buf generate buf.build/cosmos/ics23:55085f7c710a45f58fa09947208eb70b",
"gen:penumbra": "buf generate buf.build/penumbra-zone/penumbra:312294d02bf945ffa4c1b1dd7cd91328",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

empty npmignore will allow this dir to be packaged

@turbocrime turbocrime force-pushed the turbocrime/bundle-proto branch 4 times, most recently from 97af744 to 185a5db Compare August 2, 2024 10:13
Copy link
Contributor

@VanishMax VanishMax left a comment

Choose a reason for hiding this comment

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

Great improvement!

This is ultimately a better way than how it was done in #1582. Was there a specific reason to not continue working or commenting on #1582 ?

packages/protobuf/package.json Outdated Show resolved Hide resolved
@@ -2,7 +2,7 @@ import { Translator } from './types.js';
import {
SwapClaimView,
SwapClaimView_Opaque,
} from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/component/dex/v1/dex_pb.js';
} from '@penumbra-zone/protobuf/penumbra/core/component/dex/v1/dex_pb';
Copy link
Contributor

Choose a reason for hiding this comment

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

question: any chance in the future we could simplify the imports? Maybe after applying v2 of the generator we could export types as barrels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the service definitions have a barrel export defined, but i think trying to collect other types into barrels is likely to be very complex and not very useful.

@turbocrime turbocrime force-pushed the turbocrime/bundle-proto branch 2 times, most recently from 3289a2e to 5a61797 Compare August 2, 2024 18:32
@turbocrime turbocrime force-pushed the turbocrime/bundle-proto branch 2 times, most recently from 79fbeaf to 7115b4c Compare August 3, 2024 09:10
@turbocrime
Copy link
Contributor Author

going to wait until recent core message type changes are merged for ibc compat before trying to merge this

@turbocrime turbocrime changed the title bundle protobuf messages use local codegen for protobuf message types Aug 13, 2024
Comment on lines -158 to 161
const getTxHash = <T extends Required<PartialMessage<TransactionId>> | PartialMessage<Transaction>>(
t: T,
): T extends Required<PartialMessage<TransactionId>> ? string : Promise<string> =>
'inner' in t && t.inner instanceof Uint8Array
? (uint8ArrayToHex(t.inner) as T extends Required<PartialMessage<TransactionId>>
? string
: never)
: (getTxId(t as PartialMessage<Transaction>).then(({ inner }) =>
uint8ArrayToHex(inner),
) as T extends Required<PartialMessage<TransactionId>> ? never : Promise<string>);

const getTxId = (tx: Transaction | PartialMessage<Transaction>) =>
const txSha256 = (tx: Transaction | PartialMessage<Transaction>) =>
sha256Hash(tx instanceof Transaction ? tx.toBinary() : new Transaction(tx).toBinary()).then(
inner => new TransactionId({ inner }),
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this caused problems at some point during this work, and the types were technically incorrect, so it's now simplified

@turbocrime turbocrime merged commit 49263c6 into main Aug 13, 2024
6 checks passed
@turbocrime turbocrime deleted the turbocrime/bundle-proto branch August 13, 2024 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundle @buf interfaces into Protobuf
2 participants