feat: replace amino converters and proto-codecs with verana-frontend sources#268
feat: replace amino converters and proto-codecs with verana-frontend sources#268
Conversation
…sources Ref #242 Copied amino converters and proto-codecs from verana-frontend into ts-proto. - Deleted ts-proto/src/helpers/ (old monolithic aminoConverters.ts) - Created ts-proto/src/amino-converter/ with per-module files copied from verana-frontend/app/msg/amino-converter (cs, dd, perm, td, tr + util/helpers) - Replaced ts-proto/src/codec/ with verana-frontend/proto-codecs/codec/ (protoc-gen-ts_proto v1.181.2, protoc v5.29.3) - Updated test imports to use the new amino-converter paths - Added README with helper docs for writing new amino converters
|
Completed the release-safe integration path for this PR. What was done on this branch:
Frontend follow-through is done as well:
So at this point the backend package is canonical again, the prerelease is safely published, and the frontend migration has been exercised end-to-end against the published npm package. This should be ready to hand back to Maxime for final review/merge. |
genaris
left a comment
There was a problem hiding this comment.
I left a comment regarding the NPM tags.
Let's see if this works well in verana-frontend, and once we confirm that, we'll need to make sure that we'll be able to keep generating suitable proto code from this repo, without the need of adding manual patches taking code from the frontend.
.github/workflows/buildBinaries.yml
Outdated
| working-directory: ts-proto | ||
| continue-on-error: true | ||
| run: npm publish --provenance # Required for npm OIDC trusted publishing | ||
| run: npm publish --provenance --tag next # Prereleases should not move the npm latest dist-tag |
There was a problem hiding this comment.
This is true, but it is not correct for a mere Pull Request to overwrite next tag. I would consider next for commits to main branch that generate a pre-release, but not a PR, which is an ongoing work. And there could be other PRs opened as well, so it will be confusing for consumer apps that might rely on this next tag.
So my suggestion, for this case where we are working in a PR (or a maintenance branch), not to set any tag for the published NPM package.
BTW, creating pre-releases from PRs is a questionable practice, but I find it acceptable if we really need it (e.g. for asset creation that we want to download and test). I think the manual trigger is perfectly suitable for this, since we won't want to do it all the time, but only in certain, controlled cases (like this one).
There was a problem hiding this comment.
I agree that using next for a PR build is not ideal as a general rule.
Here we only did it because the frontend PR needed a package it could actually install and test before the backend PR is merged, and publishing without a tag would have pushed that build to latest, which felt worse.
If your point is just that you don’t want --tag next to stay in the workflow after this, that sounds fair to me. The main thing I needed here was a safe one-off way to publish something the frontend could test against.
Do you think there was a better way to handle this cross-PR validation flow?
There was a problem hiding this comment.
Here we only did it because the frontend PR needed a package it could actually install and test before the backend PR is merged, and publishing without a tag would have pushed that build to
latest, which felt worse.
This is not correct. Frontend doesn't need any particular tag to install this package, as long as it is published to npm. And, actually, verana-labs/verana-frontend#327 is already pointing to the exact version @verana-labs/verana-types@0.9.5-pr-268.1, so you are not using this next tag at all.
lotharking
left a comment
There was a problem hiding this comment.
There is some code duplication. It would be better to refactor this PR a bit.
| export type Exact<P, I extends P> = P extends Builtin ? P | ||
| : P & { [K in keyof P]: Exact<P[K], I[K]> } & { [K in Exclude<keyof I, KeysOfUnion<P>>]: never }; | ||
|
|
||
| if (_m0.util.Long !== Long) { |
There was a problem hiding this comment.
It would be better to move this parsing into a utility function to avoid code duplication.
There was a problem hiding this comment.
Yes, but isn't this code auto generated? I'd like to know what's the generation process for it so we can consistently create the code.
There was a problem hiding this comment.
The libraries generated by protoc should not be modified; they are generated, copied, and used exactly as protoc generates them.
There was a problem hiding this comment.
Yes, this specific file is generated.
Everything under ts-proto/src/codec/** comes from buf generate --template proto/buf.gen.ts.yaml, so I don’t think we should start hand-refactoring generated files in this PR.
The manual part of this PR is the Amino converter layer under ts-proto/src/amino-converter/** and the package exports/wiring (src/index.ts, src/signing.ts, package.json).
So on this point I agree with the principle: generated codec files should not be modified, and the custom logic should live outside them.
There was a problem hiding this comment.
If you're going to generate the proto libraries to be included in the verana-types version, you also need to specify the versions to be used. The issue indicated that the libraries from verana-front that correspond to the currently working version should be copied, which isn't the older version like the ones that were replaced. The idea isn't to generate them again, but to copy those that are known to correspond to the specified protoc version and that work correctly on the front end.
There was a problem hiding this comment.
I agree that if we generate these files, the tool and version used should be clear.
The real source of truth here is the proto files. The TypeScript codec files are only generated output from those proto definitions, so we should document the generation setup better so local generation, CI and release packaging all use the same recipe.
Where I disagree is with not generating them again. If the proto files are the source of truth, then the TypeScript package has to be generated from those proto files, not copied by hand from another repo.
This package belongs in verana because verana owns those proto definitions and needs to provide the serialization layer that matches them. It should not be up to each client to reimplement that logic separately from the same proto files.
And the frontend is not the only consumer here. The same package needs to be usable by the frontend, the indexer, the visualizer, and anything else talking to the chain.
What this branch does is bring that back into verana, and the frontend has already tested the published package successfully. That is better than keeping two copies in sync by hand.
There was a problem hiding this comment.
There are contradictions in what you've written. The goal of the verana-types library is for the amino converters and proto codecs to be packaged and used by the front end and any application that requires them. In other words, there will be no need for local generation; only the published library will be used.
Now, the point is that to maintain the library, there are two possibilities:
- The proto codecs are generated locally and tested, as are the adjustments that will likely need to be made to the Amino converters. All of this is then uploaded to generate a new version of the library with the required compatibility.
- The proto codecs are not uploaded to the repository but are instead generated in the processes that run when the library is created. In this case, it's necessary that these processes ensure the version of the protoc they use for generation is the same as the one used to make the amino converter adjustments. Currently protoc-gen-ts_proto v1.181.2 and protoc v5.29.3 (libprotoc 29.3), as indicated in the issue. In this case, the proto codecs would not be part of the PR; they would be generated during packaging.
There was a problem hiding this comment.
Thanks, this is clearer.
I think this PR is following your option 1, not option 2.
In this repo today, the generated codec files are part of the repository, they are regenerated in CI, and they are tested together with the Amino layer before publishing. That is why the codec files are part of this PR.
I also agree that the Amino converters may need manual adjustments when proto changes affect signing behavior. That is exactly why I am separating the generated codec files from the handwritten Amino layer.
I don’t think option 2 is a good fit here. If the generated codec files only appear at packaging time, then the real published output is no longer fully visible in the PR itself. That makes it harder to review, harder to test end to end, and harder to know later whether a bug came from the proto changes, the generated codec output, the Amino layer, or the packaging step.
So for this PR, I think the practical answer is:
- keep the proto files as the source of truth
- keep the generated codec files in the repo
- make the generation setup clearer
- publish the tested package from that checked-in state
antoniobenavidesvallejo
left a comment
There was a problem hiding this comment.
If you're going to generate the proto libraries to be included in the verana-types version, you also need to specify the versions to be used. The issue indicated that the libraries from verana-front that correspond to the currently working version should be copied, which isn't the older version like the ones that were replaced. The idea isn't to generate them again, but to copy those that are known to correspond to the specified protoc version and that work correctly on the front end.
|
@genaris I made the change to use a temporary tag |
Ref #242
Copied amino converters and proto-codecs from verana-frontend into ts-proto, targeting v0.9 so we can ship a compatible version with the current frontend before forward-porting to main for V4.
Build passes clean. Test type-check has 2 pre-existing errors on v0.9 (trustRegistryId in setupCreateRootPermissionPrerequisites.ts), none introduced by this PR.
Replaces bad PR from fork #266