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

multi: add dcr wallet client. #49

Merged
merged 7 commits into from
Nov 11, 2019
Merged

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Oct 15, 2019

Work towards #36.

@buck54321
Copy link
Member

Consider renaming this PR to "wallet: add contract script functions" so we can get it in. I don't want to block wallet development while you're working on DEX API pieces.

@dnldd dnldd marked this pull request as ready for review November 4, 2019 16:31
@dnldd
Copy link
Member Author

dnldd commented Nov 4, 2019

will be adding to the initial with new commits.

@buck54321
Copy link
Member

Definitely on the right track, though the Client will need quite a bit of work still. I'm going to push the BTC wallet client in a couple days, and there will be substantial changes needed for them to have a common functional interface. I can see two ways of handling this.

  1. Let's get this PR reviewed and merged, and I'll adapt the Client separately.

  2. Leave this PR idle for a couple days and we can look at the two wallets side-by-side.

I'm leaning towards #1, but I'm not crazy about merging the Client in it's current state. What do you think, @chappjc?

@chappjc
Copy link
Member

chappjc commented Nov 5, 2019

@buck54321 Either way is OK with me, although leaving this opened until the btc wallet client is up might benefit both PRs, although that's likely to result in more iterations to get them both merged. Your call.

@buck54321
Copy link
Member

I think we should merge it after review. Let's not fret too much about tests, since they would likely need substantial rewrites after the interface is reworked anyway.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Lots of work needed, but some good stuff here. I'll bring it all together with the BTC wallet. Ideally we would never merge something in this state of completion, but since I'm taking over this work and this code will help, I'm thinking we can do it this time.

@chappjc chappjc merged commit e3fa6ad into decred:master Nov 11, 2019
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