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

App child key derived from wallet master key #736

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

frnandu
Copy link
Contributor

@frnandu frnandu commented Oct 14, 2024

fixes #724

@frnandu frnandu changed the title app child key derived from wallet master key [WiP] app child key derived from wallet master key Oct 14, 2024
db/models.go Outdated Show resolved Hide resolved
nip47/event_handler.go Outdated Show resolved Hide resolved
nip47/event_handler.go Outdated Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
scopes []string,
isolated bool,
metadata map[string]interface{},
walletChildPrivKeyGeneratorFunc func(appId uint32) (string, error),
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: rather than passing this method as an argument, the keys could be passed to the db_service in the NewDBService function

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 tried that but get:

package command-line-arguments
	imports github.com/getAlby/hub/http
	imports github.com/getAlby/hub/alby
	imports github.com/getAlby/hub/config
	imports github.com/getAlby/hub/db
	imports github.com/getAlby/hub/service/keys
	imports github.com/getAlby/hub/config: import cycle not allowed

Is is possible to easily fix this import cycle without too much refactoring?

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be fixed in #710 which I move the app management out of the db package. This will cause some conflicts here when it's merged, which I can then resolve.

db/db_service.go Outdated Show resolved Hide resolved
db/models.go Outdated Show resolved Hide resolved
db/db_service.go Outdated Show resolved Hide resolved
service/keys/keys.go Outdated Show resolved Hide resolved
service/keys/keys.go Outdated Show resolved Hide resolved
service/start.go Outdated

func (svc *service) startAllExistingAppsWalletSubscriptions(ctx context.Context, relay *nostr.Relay) {
var apps []db.App
result := svc.db.Where("wallet_child_pubkey != ?", "").Find(&apps)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this query correct, shouldn't it be wallet_child_pubkey IS NOT NIL?

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'm actually not sure, because if for some reason an app DB row is created but then it fails to update the column with the calculated pubkey it will have an empty string '' as value, which the current query filters, and IS NOT NULL does not.
Is there a way to make sure that when a new DB row is created without passing a value to that column it will have NULL value instead of ''?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gorm uses the default value of the column type. If it was a *string instead of a string it would work. But maybe we will ensure all the apps have keys, so this is no longer an issue.

service/start.go Outdated Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
nip47/event_handler.go Outdated Show resolved Hide resolved
service/keys/keys.go Outdated Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
@frnandu frnandu changed the title [WiP] app child key derived from wallet master key App child key derived from wallet master key Oct 21, 2024
@frnandu frnandu marked this pull request as ready for review October 22, 2024 10:50
db/db_service.go Show resolved Hide resolved
nip47/event_handler.go Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
service/start.go Outdated Show resolved Hide resolved
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.

Separate wallet key for each connection
3 participants