Skip to content

Sync service#777

Open
GabrielMartinezRodriguez wants to merge 19 commits intomasterfrom
gabriel/sync-config
Open

Sync service#777
GabrielMartinezRodriguez wants to merge 19 commits intomasterfrom
gabriel/sync-config

Conversation

@GabrielMartinezRodriguez
Copy link
Contributor

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented May 16, 2025

Linked Issues

Description

This PR is the first version of the sync service, which implements a backend to handle CRUD operations and includes modifications in the iframe to synchronize permissions and watched assets

  • Include all relevant context (but no need to repeat the issue's content).
  • Draw attention to new, noteworthy & unintuitive elements.
Toggle Checklist

Checklist

Basics

  • B1. I have applied the proper label & proper branch name (e.g. norswap/build-system-caching).
  • B2. This PR is not so big that it should be split & addresses only one concern.
  • B3. The PR targets the lowest branch it can (ideally master).

Reminder: PR review guidelines

Correctness

  • C1. Builds and passes tests.
  • C2. The code is properly parameterized & compatible with different environments (e.g. local,
    testnet, mainnet, standalone wallet, ...).
  • C3. I have manually tested my changes & connected features.

< INDICATE BROWSER, DEMO APP & OTHER ENV DETAILS USED FOR TESTING HERE >

< INDICATE TESTED SCENARIOS (USER INTERFACE INTERACTION, CODE FLOWS) HERE >

  • C4. I have performed a thorough self-review of my code after submitting the PR,
    and have updated the code & comments accordingly.

Architecture & Documentation

  • D1. I made it easy to reason locally about the code, by (1) using proper abstraction boundaries,
    (2) commenting these boundaries correctly, (3) adding inline comments for context when needed.
  • D2. All public-facing APIs are documented in the docs.
    Public APIS and meaningful (non-local) internal APIs are properly documented in code comments.
  • D3. If appropriate, the general architecture of the code is documented in a code comment or
    in a Markdown document.
  • D4. An appropriate Changeset has been generated (and committed) with make changeset for
    breaking and meaningful changes in packages (not required for cleanups & refactors).

Copy link
Contributor Author

GabrielMartinezRodriguez commented May 16, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title feat(setting-service): first version Settings service May 16, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez added the draft Not ready for review label May 16, 2025
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 16, 2025

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: f7764cc
Status: ✅  Deploy successful!
Preview URL: https://4795d85d.happychain.pages.dev
Branch Preview URL: https://gabriel-sync-config.happychain.pages.dev

View logs

hex += b.toString(16).padStart(2, '0');
}
return '0x' + hex;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems to be leftover code to be removed, but in any case, viem has a function for this.

"compilerOptions": {
"strict": true
},
"include": ["src", "./package.json"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"include": ["src", "./package.json"]
"include": ["src"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need it because it’s used in index.ts to extract the package version

Comment on lines +3 to +5
"compilerOptions": {
"strict": true
},
Copy link
Collaborator

@norswap norswap May 21, 2025

Choose a reason for hiding this comment

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

Suggested change
"compilerOptions": {
"strict": true
},

It's already in the base file.
If you copied this over from somewhere, can you fix that over there too?

Comment on lines +3 to +5
"compilerOptions": {
"strict": true
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"compilerOptions": {
"strict": true
},

Comment on lines +8 to +11
export const logger = Logger.create("SettingsService")

const responseLogger = Logger.create("Response", LogLevel.TRACE)
export const logJSONResponseMiddleware = createMiddleware(async (c, next) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const logger = Logger.create("SettingsService")
const responseLogger = Logger.create("Response", LogLevel.TRACE)
export const logJSONResponseMiddleware = createMiddleware(async (c, next) => {
export const logger = Logger.create("SettingsService")
const responseLogger = Logger.create("Response", LogLevel.TRACE)
export const logJSONResponseMiddleware = createMiddleware(async (c, next) => {


export function isUUID(str: string): str is UUID {
return validate(str) && version(str) === 4
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should move to the common package.

"devDependencies": {
"@happy.tech/happybuild": "workspace:0.1.1",
"typescript": "^5.6.2",
"hono-openapi": "^0.4.4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate from dependencies.

include ../../makefiles/bundling.mk
include ../../makefiles/help.mk

start: ## Starts the settings service
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
start: ## Starts the settings service
dev: ## Starts the settings service

For consistency with other packages.

exports: [".", "./migrate"],
bunConfig: {
minify: false,
target: "node",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to run this with bun? Currently the submitter uses bun, and I think the other services use node. Did we have a specific reason to avoid bun at the time? I think maybe Mikro-ORM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the reason was that Bun isn't compatible with Mikro-ORM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(empty)

// The app to which the permission is granted.
invoker: AppURL
// This is the EIP-1193 request that this permission is mapped to.
parentCapability: "eth_accounts" | string // TODO only string or make specific
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's just make this string + at the site this was coming from and remove the TODO

*
* This type is copied from Viem (eip1193.ts) but we add a user field.
*/
export type WalletPermissionTable = {
Copy link
Collaborator

@norswap norswap May 21, 2025

Choose a reason for hiding this comment

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

Suggested change
export type WalletPermissionTable = {
export type WalletPermission = {

Or possibly WalletPermissionRow if you want to use WalletPermission separately, but actually I don't think rows from the DB will be loaded with this type? So maybe not WalletPermisisonRow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

dtos as "data transfer objects"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Comment on lines +13 to +18
const parsedEnv = envSchema.safeParse(process.env)

if (!parsedEnv.success) {
console.log(parsedEnv.error.issues)
throw new Error("There is an error with the server environment variables")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const parsedEnv = envSchema.safeParse(process.env)
if (!parsedEnv.success) {
console.log(parsedEnv.error.issues)
throw new Error("There is an error with the server environment variables")
}
const parsedEnv = envSchema.parse(process.env)

The default exception it throws has a good information on what went wrong during the parse, the extra code just suppressed this here.

serve({
port: env.APP_PORT,
fetch: app.fetch,
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super curious why we don't need this in the bun version. cc @not-reed
I don't think it's just a bun thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's weird. If you read the documentation of the node-server (https://github.com/honojs/node-server), they mention that this is needed for compatibility reasons with Node. However, we're running this service with Bun, so I removed it

Comment on lines +9 to +19
return {
user: permission.user,
invoker: permission.invoker,
parentCapability: permission.parentCapability,
caveats: JSON.stringify(permission.caveats),
date: permission.date,
id: permission.id,
updatedAt: permission.updatedAt,
createdAt: permission.createdAt,
deleted: permission.deleted,
}
Copy link
Collaborator

@norswap norswap May 21, 2025

Choose a reason for hiding this comment

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

Suggested change
return {
user: permission.user,
invoker: permission.invoker,
parentCapability: permission.parentCapability,
caveats: JSON.stringify(permission.caveats),
date: permission.date,
id: permission.id,
updatedAt: permission.updatedAt,
createdAt: permission.createdAt,
deleted: permission.deleted,
}
return {
...permission,
caveats: JSON.stringify(permission.caveats),
}

Isn't this approach better? Ditto for below.

.addColumn("parentCapability", "text")
.addColumn("caveats", "jsonb")
.addColumn("date", "integer")
.addColumn("id", "text", (col) => col.notNull())
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should make this a primaryKey.

And shouldn't a lot (most?) of the other fields be non-null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the ID, yes, we can do it. But for the other fields, if we make them non-null, the problem is that if we later want to make them nullable, SQLite doesn't allow altering the column. That happened to me in the randomness service, so I think we don't gain much by adding the non-null restriction. I think that it's better to enforce it in the service logic, and if we ever want to change the rule, we can do so more easily

},
persist: {
plugin: ObservablePersistLocalStorage,
name: 'config-legend',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the name of the local storage key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Comment on lines -8 to +30
const permissionsMap = useAtomValue(permissionsMapAtom)
const account = useAccount()
const appsWithPermissions = () => {
const permissions = permissionsMapLegend.get()
return Object.values(permissions).filter((permission) => !isWallet(permission.invoker)).reduce((acc, permission) => {
const existing = acc.find(([app]) => app === permission.invoker)
if (existing) {
existing[1][permission.parentCapability] = permission
} else {
acc.push([permission.invoker, {
[permission.parentCapability]: permission
}])
}
return acc
}, [] as [AppURL, AppPermissions][])
}

return entries(permissionsMap[account?.address ?? "0x0"] ?? {}) //
.filter(([app]) => !isWallet(app))
return use$(() => appsWithPermissions())
Copy link
Collaborator

@norswap norswap May 21, 2025

Choose a reason for hiding this comment

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

This confused me a good deal, but I see now it's an unfortunate consequence of the changing shape of permissionsMapLegend vs permissionsMapAtom. See my other comment in state.ts, but I think we might be able to preserve the existing shape?

Copy link
Contributor Author

@GabrielMartinezRodriguez GabrielMartinezRodriguez May 22, 2025

Choose a reason for hiding this comment

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

Actually, we can change it because the atom isn't being used directly in the iframe. It's consumed through some typed methods where I manipulate the list type to convert it to the expected version so nothing breaks. It's working fine now

return hasPermissions(app, permissionsRequest)


export const permissionsMapLegend = observable(syncedCrud({
Copy link
Collaborator

@norswap norswap May 21, 2025

Choose a reason for hiding this comment

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

The Legend State docs says that this returns by default returns a Record with keys ... so are we just free to use any key we want?

I see you return WalletPermission[] from list — but the doc says you need to you use as: ... in the object you pass to syncedCrud.

This absolutely needs a type.

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 default value returned is a record where the key is the ID of the object—that’s the type it returns. Returning an array directly isn’t an option when consuming the state in the frontend. As stated in the docs (https://legendapp.com/open-source/state/v3/sync/supabase/#as):

Note that array is not an option because arrays make it hard to efficiently and correctly add, update, and remove elements by ID.

The type is already inferred; the observable has this shape: Observable<Record<string, WalletPermission>>, where the string is the ID

As for your question about why the list method returns an array: it's because the list should provide all the permissions that have changed. Internally, Legend State then stores them as a record

Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw the name of the directory should be sync-service because it doesn't only deal in settings :)

if (app === getWalletURL()) {
// Permissions don't exist, create them.
// The iframe is always granted the `eth_accounts` permission.
const permissionId = createUUID()
Copy link
Collaborator

@norswap norswap May 21, 2025

Choose a reason for hiding this comment

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

Using the UUID as key is rather problematic, because as indicated in its comment on WalletPermission, it's not something we really use or need and so it's just filled by generated BS.

Which means it's not deterministic, which means it presents a problem for sync, as if two clients fall out of sync and have both granted connection permission (eth_accounts) to an app, then they will appear as though they are different permissions because of the different UUID.

The same case can also happen here (and this doesn't need out of sync scenarios): the permissions to the iframe are auto-granted.

We could use (user address + appURL) as key instead. That does however make the value arrays of app permissions (or even a recrod) instead of plain permissions. We could also just use the user address (first level of the map), given values of type Record<AppURL, AppPermissions> aka Record<AppURL, Record<string, WalletPermission>> — is using these kinds of values supported? Does the sync work on them? If so, we should use that (at least, at first). So we can preserve our existing schema and logic.

The real question is: what do update actually look like? if I change a field down the object graph, does it only send that field? But also can we send similarly partial objects from the server? (Well I guess we control the list function, so technically yes we can send these partial updates and apply them ourselves to the existing object in the list function)

Copy link
Collaborator

Choose a reason for hiding this comment

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

^ Most of the value of the review is in this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to use the key: const id = ${user.address}-${app}-${permissionName}, and with this change it's working fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the data structure we save is a WalletPermissions, and when we use the list method, it returns a WalletPermissions[], which is then converted to a Record<string, WalletPermissions>, where the string is the ID of the WalletPermissions. When we perform an update, we only send the specific WalletPermissions that changed

initial: {},
fieldCreatedAt: 'created_at',
fieldUpdatedAt: 'updatedAt',
fieldDeleted: 'deleted',
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you specify this, it doesn't call delete which can be omitted but update instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end, we're not using it because I changed the approach from only fetching the differences to loading the entire state each time, as the previous method was causing syncing issues. After that change, we no longer need soft deletes and can fully delete instead

Copy link
Contributor

@not-reed not-reed left a comment

Choose a reason for hiding this comment

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

didn't run it yet, but looks cool so far!


import { env } from "../env"

const dbPath = env.SETTINGS_DB_URL || ":memory:"
Copy link
Contributor

Choose a reason for hiding this comment

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

probably makes sense for the default to be set globally in .env ?

Comment on lines +7 to +8
target: "node",
external: ["better-sqlite3"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
target: "node",
external: ["better-sqlite3"],
target: "bun",

if your using bun for the runtime in prod (looks like it will be) 😈

import { z } from "zod"

const envSchema = z.object({
NODE_ENV: z.enum(["development", "production"]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
NODE_ENV: z.enum(["development", "production"]),
NODE_ENV: z.enum(["development", "production"]).default('development'),

const envSchema = z.object({
NODE_ENV: z.enum(["development", "production"]),
APP_PORT: z.string().transform((s) => Number(s)),
SETTINGS_DB_URL: z.string().trim(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SETTINGS_DB_URL: z.string().trim(),
SETTINGS_DB_URL: z.string().trim().default(':memory:'),


const envSchema = z.object({
NODE_ENV: z.enum(["development", "production"]),
APP_PORT: z.string().transform((s) => Number(s)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
APP_PORT: z.string().transform((s) => Number(s)),
APP_PORT: z.coerce.number(),

@@ -0,0 +1,23 @@
import type { ContentfulStatusCode } from "hono/utils/http-status"

export abstract class HappySettingsError extends Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have used it anywhere, but hono does have an HttpException class (seems thats basically what this is implementing?) https://hono.dev/docs/api/exception#throw-httpexception


export const deleteConfigSchema = z
.object({
id: z.string().openapi({ example: "78b7d642-e851-4f0f-9cd6-a47c6c2a572a" }),
Copy link
Contributor

Choose a reason for hiding this comment

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

i guess the id is no longer a uuid? (i think i saw that on the front end)

@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title Settings service Sync service Jun 25, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-1 Ready for, or undergoing first-line review and removed draft Not ready for review labels Jun 25, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review June 25, 2025 14:15
@linear
Copy link

linear bot commented Jun 25, 2025

HAPPY-284 Backend-driven user information sync

We will want to synchronize the user's information (see list below) between multiple instances of its wallets (tabs, devices, standalone, …).

We used to do sync localStorage-driven sync on the same device, but that's doesn't actually work when the wallet iframe is embedded under different domains. It never covered the cross-device use-case which we do want anyway.

Reference to the previous conversation on storage-based sync: HAPPY-137

Information to sync (pretty much anything that sits in storage):

  • transaction history
  • permissions
  • added chains
  • watched assets
  • added ABIs
  • session keys
    • BUT encrypted to a key derived from controlling EOA, so that setting up in a new storage context brings session keys along

What not to sync:

  • user details (come from auth)
  • local keyshare once we have that

Auth

First, user info should be private, so there is a need to auth the user. For oauth, this should probably be JWT driven. But how do we make this work for injected wallet controlled account?

  • Signing a challenge (~ Sign in with Ethereum or SIWE) works, but it's annoying because that's an extra user interaction each time we need to refresh auth.
  • Auth session key: derive a private key from a wallet signature, needed once per storage context (storage partition x browser-install). Bit better.

Where to sync

  • Firebase: but can we make this to work with key verification?
  • Jazz is a promising library that can host a DB and sync it locally.
    • but the docs are pretty WIP
    • hard to say how pluggable authentication is for our needs
  • Other / custom

How to sync

Will depend on the "where", but assuming either a DB or an API endpoint, basically send an atomic "SQL-like" update.

There's a question of whether such updates can cover all of our use-cases. Something that (1) reads the storage, then (2) issues an update based on it, could cause race conditions. I believe if the read part of the storage is written as part of the update, then that might alleviate the issue? Not 100% sure.

If that doesn't work, then we need API endpoint that perform the read + write transaction atomically on the server.

@GabrielMartinezRodriguez GabrielMartinezRodriguez mentioned this pull request Jun 27, 2025
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

reviewing-1 Ready for, or undergoing first-line review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants