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

client/{asset,core}: wallet notifications #2492

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

buck54321
Copy link
Member

Gives wallets a way to issue asynchronous notifications. Replaces the TipChange function. These notifications are sent to the front end, but are not used yet. This will help solve some issues that were raised in #2482, but it's also just a better pattern than the tip change function and should enable more up-to-date information on the front end.

@buck54321 buck54321 mentioned this pull request Aug 30, 2023
Comment on lines +5391 to +5535
// stakeInfo, err := dcr.wallet.StakeInfo(dcr.ctx)
// if err != nil {
// dcr.log.Errorf("Error getting stake info for tip change notification data: %v", err)
// } else {
// data = &struct {
// TicketPrice uint64 `json:"ticketPrice"`
// }{
// TicketPrice: uint64(stakeInfo.Sdiff),
// }
// }
dcr.emit.TipChange(uint64(height), data)
Copy link
Member Author

Choose a reason for hiding this comment

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

I leave this here as a demonstration of how wallets can use these to convey asset-customized info when needed. Another use case is for asynchronous ticket purchasing, which may be necessary when mixing is enabled (or even for unmixed tickets, possibly). The StakeInfo method isn't added in this PR, but it is in the gui staking PR #2482.

}

// Errorf formats and sends an AsyncWalletErrorNote.
func (e *WalletEmitter) Errorf(s string, a ...interface{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of only Errorf, a general Logf note with an error level argument would be useful to let core know the severity of the notification to send to the front end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you have a particular place you were thinking it was needed, because I was thinking of removing Errorf and Error altogether. The only thing Core does is log it, and the wallet already has a logger that logs to the same place but with more context. We can add Logf or anything else as needed, but I don't have a use for it right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's definitely not needed yet.

@buck54321 buck54321 merged commit f629e95 into decred:master Sep 11, 2023
5 checks passed
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.

3 participants