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.

Sign up for GitHub

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

Jump to bottom

feat: protocol-parser in go #1116

Merged
9 commits merged into from
Jul 27, 2023
Merged

feat: protocol-parser in go #1116

9 commits merged into from
Jul 27, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jul 8, 2023

Implements the same api in the most consistent way possible (including file structure) as https://github.com/latticexyz/mud/tree/main/packages/protocol-parser

@changeset-bot
Copy link

changeset-bot bot commented Jul 8, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 4d76fba

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

This PR includes changesets to release 26 packages
Name Type
@latticexyz/services Patch
@latticexyz/cli Patch
@latticexyz/network Patch
@latticexyz/std-client Patch
@latticexyz/dev-tools Patch
@latticexyz/ecs-browser Patch
@latticexyz/block-logs-stream Patch
@latticexyz/common Patch
@latticexyz/config Patch
create-mud Patch
@latticexyz/gas-report Patch
@latticexyz/noise Patch
@latticexyz/phaserx Patch
@latticexyz/protocol-parser Patch
@latticexyz/react Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
@latticexyz/solecs Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/std-contracts Patch
@latticexyz/store-cache Patch
@latticexyz/store-sync Patch
@latticexyz/store Patch
@latticexyz/utils Patch
@latticexyz/world Patch

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

@ghost ghost force-pushed the authcall/protocol-parser-go branch from c9da625 to 4734861 Compare July 9, 2023 06:26
@ghost ghost force-pushed the authcall/protocol-parser-go branch from 4734861 to 128d0db Compare July 9, 2023 17:49
@ghost ghost force-pushed the authcall/protocol-parser-go branch from 128d0db to 12368e1 Compare July 9, 2023 17:52
@ghost ghost marked this pull request as ready for review July 9, 2023 18:27
@ghost ghost requested review from alvrs and holic as code owners July 9, 2023 18:27
alvrs
alvrs reviewed Jul 13, 2023
View reviewed changes
Copy link
Member

@alvrs alvrs 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 great! just added some minor comments. for completeness, can we add a port for decodeKeyTuple?

.github/workflows/services.yml Outdated
Comment on lines 20 to 25
- name: Build
working-directory: ./packages/services
run: go build -v ./...

- name: Test
working-directory: ./packages/services
Copy link
Member

Choose a reason for hiding this comment

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

if we add this to the build and test scripts in the package.json it will automatically run when running pnpm build / pnpm test at the root, so we don't need this separate action

packages/services/pkg/protocol-parser/decodeDynamicField.go Outdated
Comment on lines 120 to 121
ADDRESS_ARRAY,
BOOL_ARRAY:
Copy link
Member

Choose a reason for hiding this comment

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

ultra tiny nit but my OCD brain wants these to be switched (first BOOL_ARRAY, then ADDRESS_ARRAY) to match the schema type enum

packages/services/pkg/protocol-parser/decodeStaticField.go
BYTES30,
BYTES31,
BYTES32:
// TODO: should we return a fixed size array per bytes size or a slice?
Copy link
Member

Choose a reason for hiding this comment

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

good q, in the ts version we return a hex string padded to the size of the bytes. In go, is it possible to pass a fixed size byte array to a function that accepts variable size byte arrays? if so, I think returning a fixed size byte array here would be a nice touch

packages/services/pkg/schema-type/arrayAbiTypes.go
package schematype

func ArrayAbiTypeToStaticAbiType(schemaType SchemaType) SchemaType {
return (schemaType - UINT8_ARRAY)
Copy link
Member

Choose a reason for hiding this comment

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

should we add a check here that the input type is actually an array type and throw an error else? thinking of cases where users incorrectly input BYTES, STRING or a static type

@alvrs
Copy link
Member

alvrs commented Jul 13, 2023

also let's add a changeset! (run pnpm changeset locally and describe this change)

@ghost ghost force-pushed the authcall/protocol-parser-go branch from f86a685 to 2261ca7 Compare July 17, 2023 17:02
@ghost ghost requested a review from alvrs July 17, 2023 17:07
dk1a
dk1a reviewed Jul 17, 2023
View reviewed changes
Copy link
Contributor

@dk1a dk1a left a comment

Choose a reason for hiding this comment

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

would it be better to move schema-type stuff from packages/services/pkg/schema-type to packages/schema-type/go?

@alvrs
Copy link
Member

alvrs commented Jul 19, 2023

would it be better to move schema-type stuff from packages/services/pkg/schema-type to packages/schema-type/go?

Agree! not blocking for this PR though imo, can also do that in a follow up

@ghost ghost merged commit 3236f79 into main Jul 27, 2023
@ghost ghost deleted the authcall/protocol-parser-go branch July 27, 2023 17:52
@github-actions github-actions bot mentioned this pull request Jul 27, 2023
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holic holic Awaiting requested review from holic holic is a code owner

Assignees
No one assigned
Labels
changes requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alvrs @dk1a