Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Start using Protobuf types #19

Draft
wants to merge 9 commits into
base: jessepinho/profile-screen
Choose a base branch
from

Conversation

jessepinho
Copy link
Collaborator

@jessepinho jessepinho commented Jan 4, 2025

At a high level, this PR is about introducing Protobuf types to the mobile app (rather than the proprietary types I was using to quickly scaffold up a UI).

@TalDerei I'm requesting a review even though this is in draft. Up to you whether to merge it and build off of it, or resolve the module resolution issues first.

Issues I ran into

There's something really weird going on with the @penumbra-zone/... packages. VSCode would complain that it couldn't resolve the e.g. @penumbra-zone/protobuf modules. I had to add "type": "module" to our package.json, and then add "moduleResolution": "bundler" to tsconfig.json. While that made VSCode happy, I was still getting build failures from Expo, so I also needed to add aliases to metro.config.cjs (which I also had to rename from its previous .js extension). The aliases specifically are super hacky, and not really a long-term solution, so I unfortunately have to leave this up to y'all to resolve as I won't be able to fix these module resolution issues in time. I'm not sure why it's happening only with our @penumbra-zone packages, but I'm guessing we're somehow publishing them incorrectly.

One other issue I ran into: Redux complains about non-serializable values in state due to the BigInts we're storing in Redux. This is going to need to be addressed head-on, since we use BigInts everywhere. I was hoping there was some clean transform middleware for Redux where you can serialize/deserialize problematic values, but I'm not sure it'll be that simple. Needs some investigation.

All that said, here's what I did accomplish...

In this PR

  • Introduce reselect to memoize slower-to-calculate selectors.
  • Use Protobuf BalancesResponses instead of my custom Balance type.
  • Use Protobuf Addresses instead of my custom Address type.
  • Update factories to use Protobuf types.

@jessepinho jessepinho force-pushed the jessepinho/protobufs branch from bef7ca4 to 6be2224 Compare January 4, 2025 05:33
@jessepinho jessepinho force-pushed the jessepinho/profile-screen branch from 6f47cd1 to b53d703 Compare January 6, 2025 20:32
@jessepinho jessepinho force-pushed the jessepinho/protobufs branch from 4d40d9b to 7cdb47c Compare January 6, 2025 20:34
@jessepinho jessepinho requested a review from TalDerei January 6, 2025 20:50
@jessepinho
Copy link
Collaborator Author

@TalDerei commits from this PR (for when you need to rebase after profile-screen is merged).

image


## Weird issues/gotchas

### Importing `@penumbra-zone/*` packages
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually don't think this is a solution at all, and this PR probably shouldn't be merged in its present state, but I'll leave that up to y'all to decide. I couldn't get the package imports to work without these metro.config changes (along with the tsconfig/package.json changes).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant