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

wallet: remove force option from wallet.makeOpen. #815

Merged
merged 1 commit into from
May 29, 2023

Conversation

nodech
Copy link
Contributor

@nodech nodech commented May 25, 2023

force option is leftover after refactors as far as I can see in the history.
As it is not used and no one knows why it was there, might as well clean up.

@nodech nodech added quick review difficulty - easy wallet part of the codebase cleanup improvement classification labels May 25, 2023
@coveralls
Copy link

Coverage Status

Coverage: 68.423% (-0.02%) from 68.44% when pulling 2f0e520 on nodech:wallet-open-cleanup into 2bb8f0b on handshake-org:master.

@nodech nodech added patch Release version breaking-major Backwards incompatible - Release version and removed patch Release version labels May 25, 2023
Copy link
Member

@rithvikvibhu rithvikvibhu left a comment

Choose a reason for hiding this comment

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

Closes #93!

Comment on lines +7 to +10
- HTTP Changes:
- `/wallet/:id/open` no longer accepts `force` flag. (it was not used)
- RPC Changes:
- `createopen` and `sendopen` no longer accept `force` as an argument. (was not used)
Copy link
Member

Choose a reason for hiding this comment

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

It feels repetitive, but since hsd can used as a library, does it make sense to also add wallet.{create,send}Open() change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It sounds too much to me. It would be good to document every change like that for the lower level consumers of the hsd, but given the ability to reorganize/refactor some code that does not break intended API that is HTTP/RPC only - gives us more flexibility.

I would consider changes to MTX/TX/Script etc primitives that are almost necessary in some front-end cases (e.g. working with ledger etc) to be part of the breaking changes.

Given wallet is meant to be consumed via HTTP, I think documenting this as a breaking change to become more troublesome. If we were to go that way, every change to any class should be documented here as well, as more involved use cases may use those.

So, in summary, I would stick to the current rules: document breaking API (HTTP/RPC/DNS) and MAYBE primitives.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍

@nodech
Copy link
Contributor Author

nodech commented May 29, 2023

Rebased

@nodech nodech merged commit 5fe70c0 into handshake-org:master May 29, 2023
5 checks passed
@nodech nodech deleted the wallet-open-cleanup branch May 29, 2023 14:06
nodech added a commit that referenced this pull request Jul 17, 2023
@nodech nodech mentioned this pull request Jul 17, 2023
This was referenced Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-major Backwards incompatible - Release version cleanup improvement classification quick review difficulty - easy wallet part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants