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

Lex refactor branch ("last big breaking lexicon changes") #93

Merged
merged 64 commits into from
Apr 1, 2023

Conversation

whyrusleeping
Copy link
Collaborator

Working towards #83 mostly

bigsky and laputa compile, havent tested them yet. Still a fair emount left to do

@bnewbold bnewbold force-pushed the feat/lexicon-update branch from 96b71b8 to be991d1 Compare March 28, 2023 17:38
@bnewbold
Copy link
Collaborator

bnewbold commented Mar 28, 2023

A couple notes from getting things running:

  • everything now compiles and tests pass
  • com.atproto.server.getAccountsConfig was missing from the generated PDS stubs file. Not sure why, maybe a bug in lexgen? (UPDATE: this endpoint was renamed com.atproto.server.describeServer
  • the nice RegisterHandlersComAtproto helper function in BGS isn't being used; instead the endpoints are manually created
  • did a rebase on top of main (to resolve a merge conflict in gosky)
  • I suspect that there are more changes needed. Don't think the new $link and $bytes JSON schema is implemented

@bnewbold
Copy link
Collaborator

I guess I spoke too soon: the tests pass locally for me (including in a fresh checkout), but not in CI. It is possible that golang 1.19 vs. 1.20 is the issue (I've updated on my laptop; this PR hasn't landed: #91). But more likely some timing issue.

@bnewbold
Copy link
Collaborator

Bumping a sleep in one of the integration tests from 50ms to 100ms seems to have helped with the CI test failures.

I did a large round of updating lexicons from upstream (there have been changes to lex-refactor branch in atproto, and may be a few more before everything gets merged), including removing deprecated stuff this time around.

I hacked in $link syntax for cid-link, but only for lexutil.Blob. We should probably handle that better, maybe with a wrapper type like LexCidLink? It is nice just having cid.Cid "right there".

Not 100% confident that I updated all the XRPC handlers and stubs to new/correct values, and there might be some deprecated/dead code in there. Did do some greps for all the older/deleted names.

I started updating subscription event schemas, aka federation wire protocol. I thought that event kind numbers changed to strings in the lex refactor, but I didn't make that change yet. BGS repo subscriptions are not working against atproto implementation yet, failing with invalid signatures. Integration tests within indigo do pass, and labelmaker subscribes to atproto PDS ok.

@bnewbold bnewbold changed the title The last big lexicon refactors Lex refactor branch ("last big breaking lexicon changes") Mar 29, 2023
@bnewbold
Copy link
Collaborator

bnewbold commented Mar 30, 2023

Some more progress, all tests are passing now. BGS interop with typescript lex-refactor branch is not working yet.

I hacked in the subscription changes pretty quickly, I suspect that more refactoring might be needed in BGS, repomgr, etc. The previous RepoAppend object could be both a repo_append or a rebase, based on the Event field value; now the different event types are different structs. Maybe we should pass around entire XRPCStreamEvent structs? This is just a vague impression, I just "got things to compile" without digging in to how the rebase vs. commit code paths work.

Tweaked a couple lexgen things relating to required-but-nullable fields. This is only implemented on structs for now, not handler function signatures or query params.

Created an issue to track post-refactor follow-ups: #95

I have hit a couple different errors trying to do BGS interop with typescript implementation. One error looks like:

2023-03-29T21:16:33.850-0700	ERROR	indexer	indexer/crawler.go:160	failed to perform repo crawl of "did:plc:lzkaig7u5entvhqgr4cgabwm": importing fetched repo (curHead: bafyreia7y77ta5i3sazxkkpfmjl5j2adznebxjbps4odgmryrl2v3qffji): process new repo (current head: bafyreia7y77ta5i3sazxkkpfmjl5j2adznebxjbps4odgmryrl2v3qffji): walkTree: can only handle dag-cbor objects in repos (bafkreif6df2wrudglcyzdcx5qcjvv6e6lmipfzghvdzxscl5klgbu4lu6y is 85):

I think that is the indexer code breaking on the new CID links in records, which in the case of blobs are "raw" CID pointers to file bytes (not dag-cbor multicodec). The only other current use of the cid-link datatype is in repo subscription objects, but in the future there might be more CIDs in records pointing to either dag-cbor (in local or remote repos), or raw CIDs (pointing to arbitrary content).

Another error is:

2023-03-29T21:50:03.697-0700	ERROR	bgs	bgs/fedmgr.go:168	failed handling event from "localhost:2583" (252): handle user event failed: signature check failed: invalid signature

I'm not sure what the deal is with that. We have an existing test in testing/sig_test.go for basic signature validation.

New users don't seem to get picked up (or at least logged) by BGS; labelmaker event consumers sees them and ignores. This was a log-level thing, bigsky seems to hard-code logging to "WARN" level.

@bnewbold
Copy link
Collaborator

Ah, as a hint, also seeing the signature failure against typescript when using 'main' branch on both.

@bnewbold bnewbold force-pushed the feat/lexicon-update branch from 8a292b8 to 452ca53 Compare March 30, 2023 21:51
@whyrusleeping
Copy link
Collaborator Author

Got the embed/union issue figured out. That test still isnt passing yet but its better.

@bnewbold bnewbold merged commit d0ea072 into main Apr 1, 2023
@bnewbold bnewbold deleted the feat/lexicon-update branch April 1, 2023 03:14
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.

2 participants