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

feat: Waku v2 bridge #1

Closed
wants to merge 1 commit into from
Closed

feat: Waku v2 bridge #1

wants to merge 1 commit into from

Conversation

endulab
Copy link

@endulab endulab commented Dec 8, 2023

Issue #12610

image

@endulab endulab self-assigned this Dec 8, 2023
@endulab endulab linked an issue Dec 8, 2023 that may be closed by this pull request
@endulab endulab marked this pull request as ready for review January 16, 2024 10:32
@endulab endulab requested a review from jakubgs January 16, 2024 10:32
@endulab
Copy link
Author

endulab commented Jan 16, 2024

@endulab endulab changed the title feat: Waku v2 bridge - WIP feat: Waku v2 bridge Jan 16, 2024
"EnableDiscV5": true,
"DiscoveryLimit": 20,
"AutoUpdate": true,
"PeerExchange": true
Copy link
Member

Choose a reason for hiding this comment

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

Since this will not run as light mode, probably makes sense to disable peer exchange (discv5 is enabled so peer discovery will still work)

fleet.json Outdated
"Enabled": false
},
"LogEnabled": true,
"LogLevel": "DEBUG"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be INFO?

configFiles,
)
if err != nil {
b.Log.WithError(err).Error("Failed to generate config")
Copy link
Member

Choose a reason for hiding this comment

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

Should it return or panic?


createAccRequest := &requests.CreateAccount{
WalletSecretsConfig: requests.WalletSecretsConfig{
InfuraToken: os.Getenv("STATUS_BUILD_INFURA_TOKEN"),
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the environment variable is not set? Will the returned configuration still be valid?

Comment on lines 239 to 255
const communityIdLength = 68
return chatId[0:communityIdLength]
Copy link
Member

Choose a reason for hiding this comment

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

Would it be a good idea to check if the chatId length is >= communityIdLength and fail otherwise?

return "", fmt.Errorf("bridge %s not connected, dropping message %#v to bridge", b.Account, msg)
}

b.Log.Infof("=> Sending message %#v", msg)
Copy link
Member

Choose a reason for hiding this comment

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

should this be Debug?

Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

Nice work. But boy do I love reviewing PRs with vendor folders...

image

"ClusterConfig": {
"ClusterID": 16,
"Enabled": true,
"Fleet": "shards.test",
Copy link
Member

Choose a reason for hiding this comment

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

Is use of shards.test temporary or intentional and we plan to provide other options like status.prod in the future?

bridge/status/status.go Outdated Show resolved Hide resolved
Comment on lines +405 to +407
func (b *Bstatus) JoinChannel(channel config.ChannelInfo) error {
return b.joinCommunityChannel(channel)
Copy link
Member

Choose a reason for hiding this comment

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

What's the point of having a JoinChannel that is just a wrapper around joinCommunityChannel? I don't get it.

Copy link
Author

Choose a reason for hiding this comment

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

There is no specific reason.
At some point I have 2 functions: joinCommunityChannel and joinPrivateChat.

Comment on lines +308 to +311
walletDB, err := walletdatabase.InitializeDB(b.statusDataDir+"/"+"wallet.db", "", dbsetup.ReducedKDFIterationsNumber)
if err != nil {
return errors.Wrap(err, "Failed to initialize wallet db")
}
b.statusNode.SetWalletDB(walletDB)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a wallet DB file? Can't this be in memory or not used at all? Kinda weird.

Copy link
Author

Choose a reason for hiding this comment

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

CLI app also have it. I need to test if removing it has no impact on joining token-gated communities.

Issue #12610
@jakubgs
Copy link
Member

jakubgs commented Mar 11, 2024

Why was this closed?

@endulab
Copy link
Author

endulab commented Mar 11, 2024

Why was this closed?

I want waku2 remain a developement branch for waku v2 integration.
master is a pure copy of 42wim/matterbridge repo.
Let me know if this is ok for you.

@jakubgs
Copy link
Member

jakubgs commented Mar 12, 2024

Sure. But considering the purpose of this repo, should we make waku2(shouldn't it be wakuv2?) the default branch?

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.

[Bridge] Bridge status communities to Discord
3 participants