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.

Sign up for GitHub

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

Jump to bottom

feat: Add -chainlocknotify cmd-line option, update -instantsendnotify #5522

Merged
merged 4 commits into from
Aug 15, 2023
Merged

feat: Add -chainlocknotify cmd-line option, update -instantsendnotify #5522

merged 4 commits into from
Aug 15, 2023

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Aug 2, 2023

Issue being fixed or feature implemented

Execute command when the best chainlock changes (%s in cmd is replaced by chainlocked block hash). Same as -blocknotify but for chainlocks. Let -instantsendnotify replace %w with wallet name like -walletnotify does.

What was done?

How Has This Been Tested?

run tests

Breaking Changes

n/a

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@UdjinM6 UdjinM6 added this to the 20 milestone Aug 2, 2023
knst
knst reviewed Aug 2, 2023
View reviewed changes
Copy link
Collaborator

@knst knst left a comment •
edited
Loading

Choose a reason for hiding this comment

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

why do we need to create a brand new functional test with mostly copy-paste code?

It seems as possible to extend old test test/functional/feature_notifications.py with manageable amount of efforts, or do I miss some important difference?

test/functional/feature_notifications_dash.py Outdated Show resolved Hide resolved
src/wallet/init.cpp Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

This pull request has conflicts, please rebase.

@UdjinM6
feat: Add -chainlocknotify cmd-line option
31b6ce7 
Execute command when the best chainlock changes (%s in cmd is replaced by chainlocked block hash).
@UdjinM6
Copy link
Author

UdjinM6 commented Aug 2, 2023

why do we need to create a brand new functional test with mostly copy-paste code?

I wouldn't agree with mostly copy-paste code part. I only borrowed like 3 variables and 1 function. :)

It seems as possible to extend old test test/functional/feature_notifications.py with manageable amount of efforts, or do I miss some important difference?

That's how it would look like d1461b0. Had to implement a few workarounds to make it work, will most likely cause merge conflicts in the future.

knst
knst reviewed Aug 3, 2023
View reviewed changes
Copy link
Collaborator

@knst knst left a comment •
edited
Loading

Choose a reason for hiding this comment

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

That's how it would look like d1461b0. Had to implement a few workarounds to make it work,

yes, indeed, I counted 3 of them:

  • self.nodes[1] -> rpc
  • different extra_args logic (because we have non-empty args by default)
  • different way to create wallets & removing files

Despite that I think it is still better that own file for feature_notifications_dash.py by 2 reasons:

  • potential conflicts don't look as really difficult to resolve in this particular case because the overall architecture and logic is not changed: all changes are in details of implementation
  • if it would be separate script feature_nofications_dash.py then no any changes from original feature_notifications.py would be ever backported to feature_nofications_dash.py -> over time difference between these scripts would be bigger and bigger and potentially it increase complexity of support over time.

Anyway, I am not strongly against of separate script, but I'd prefer having them together.

Btw, why do we need to mine 4 quorums here? It works even with one quorum (as I tested locally). Otherwise LGTM now.

test/functional/feature_notifications.py Outdated Show resolved Hide resolved
@UdjinM6
Copy link
Author

UdjinM6 commented Aug 3, 2023

Dropped feature_nofications_dash.py, squashed all fixes. Also added e7236f0 which should make diff smaller by avoiding "self.nodes[1] -> rpc" part.

knst
knst approved these changes Aug 3, 2023
View reviewed changes
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK

thephez
thephez approved these changes Aug 3, 2023
View reviewed changes
Copy link
Collaborator

@thephez thephez left a comment

Choose a reason for hiding this comment

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

Slightly-tested ACK (ran with ... -chainlocknotify="notify-send New ChainLock")

image

PastaPastaPasta
PastaPastaPasta approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for squash merge

@PastaPastaPasta PastaPastaPasta merged commit 9f7322b into dashpay:develop Aug 15, 2023
1 check passed
@PastaPastaPasta PastaPastaPasta added the Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged. label Aug 30, 2023
@UdjinM6 UdjinM6 deleted the chainlocknotify branch August 30, 2023 18:32
knst added a commit to knst/dash that referenced this pull request Oct 22, 2024
@knst
refactor: unify feature_notifications.py after dashpay#5522 with bitc…
11e77c2 
…oin's codebase
knst added a commit to knst/dash that referenced this pull request Oct 24, 2024
@knst
refactor: unify feature_notifications.py after dashpay#5522 with bitc…
ad2c5a5 
…oin's codebase
knst added a commit to knst/dash that referenced this pull request Oct 24, 2024
@knst
refactor: unify feature_notifications.py after dashpay#5522 with bitc…
7fae18e 
…oin's codebase
PastaPastaPasta added a commit that referenced this pull request Oct 25, 2024
@PastaPastaPasta
Merge #6357: backport: bitcoin#22229 test: consolidate to f-strings a…
4b5e392 
…nd related fixes

7c6c93d fix: remove missing comment to follow-up for bitcoin#15864 (Konstantin Akimov)
65226da Merge bitcoin#22229: test: consolidate to f-strings (part 1) (MarcoFalke)
ad2c5a5 refactor: unify feature_notifications.py after #5522 with bitcoin's codebase (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Just one backport bitcoin#22229 because it is a big size, even though there's nothing non-trivial in it.

  Though, even it is called as part I, there has not been part II yet.

  ## What was done?
  Some preparation, code unifications to make bitcoin#22229 with less conflicts and finally backport of itselfl.

  ## How Has This Been Tested?
  Run unit & functional test

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 7c6c93d
  PastaPastaPasta:
    utACK 7c6c93d

Tree-SHA512: fe296e3255d45a7a1924bd1e5e21634b3cd36ea3f71cf5e8684b54336771665ea7758de7bfc78721669a928f967e7d4db7b1da0a5cd275feb1a2ec0df841ad5c
knst added a commit to knst/dash that referenced this pull request Oct 28, 2024
@knst
refactor: unify feature_notifications.py after dashpay#5522 with bitc…
96df996 
…oin's codebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thephez thephez thephez approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@knst knst knst approved these changes

Assignees
No one assigned
Labels
Needs Release Notes This PR includes breaking changes for which release notes have not yet been merged.
Projects
None yet
Milestone
20
Development

Successfully merging this pull request may close these issues.
4 participants
@UdjinM6 @knst @PastaPastaPasta @thephez