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

server: use decred's rpcclient for BTC #975

Merged
merged 12 commits into from
May 4, 2021

Conversation

amass01
Copy link
Member

@amass01 amass01 commented Feb 14, 2021

That's the second part of and closes #841, #938 was the first part and it
migrated the client to use decred's v6 context aware rpcclient.

@amass01 amass01 marked this pull request as ready for review February 17, 2021 01:50
@amass01 amass01 changed the title [wip] server: use decred's rpcclient for BTC. server: use decred's rpcclient for BTC. Feb 17, 2021
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

I had the simnet tests fail a few times while testing this, but not sure if related to these changes.

server/asset/btc/rpcclient.go Show resolved Hide resolved
server/asset/btc/btc.go Outdated Show resolved Hide resolved
server/asset/btc/btc.go Outdated Show resolved Hide resolved
@amass01 amass01 force-pushed the rpcclientserver branch 2 times, most recently from 6560641 to db499d8 Compare February 25, 2021 17:09
@chappjc chappjc changed the title server: use decred's rpcclient for BTC. server: use decred's rpcclient for BTC Mar 1, 2021
@chappjc chappjc self-requested a review March 1, 2021 14:11
@amass01
Copy link
Member Author

amass01 commented Mar 8, 2021

@chappjc had to adjust: 8d43164 after rebasing onto master.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Does cloneCfg.FeeEstimator/estimateFee's signature work as estimateFee func(context.Context, RawRequester) (uint64, error)? I'm not sure but maybe this would avoid a lot of exporting?

@amass01
Copy link
Member Author

amass01 commented Mar 10, 2021

@JoeGruffins Thanks for the suggestion, was sure it's not idiomatic.

@chappjc
Copy link
Member

chappjc commented Mar 10, 2021

You could also define a wrapper for callers that are not context-enabled so that it just uses the btc.Backend's subsystem shutdown context

func (rc *RPCClient) RawRequest(string, []json.RawMessage) (json.RawMessage, error){
    return rc.Requester.RawRequest(rc.ctx, method, params)
}

So that BCH can do

func estimateFee(node btc.RPCClient) (uint64, error) {
	resp, err := node.RawRequest("estimatefee", nil)

instead of

func estimateFee(node btc.RPCClient) (uint64, error) {
	resp, err := node.Requester.RawRequest(node.Ctx, "estimatefee", nil)

@amass01
Copy link
Member Author

amass01 commented Mar 10, 2021

@chappjc will do 💯

@amass01
Copy link
Member Author

amass01 commented Mar 10, 2021

@JoeGruffins @chappjc updated

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Works for me.

server/asset/btc/btc.go Outdated Show resolved Hide resolved
server/asset/btc/btc.go Outdated Show resolved Hide resolved
server/asset/btc/rpcclient.go Outdated Show resolved Hide resolved
server/asset/btc/rpcclient.go Outdated Show resolved Hide resolved
server/asset/btc/rpcclient.go Outdated Show resolved Hide resolved
server/asset/btc/rpcclient.go Outdated Show resolved Hide resolved
@chappjc chappjc added this to the 0.3 milestone Apr 7, 2021
@chappjc chappjc merged commit 91d5a37 into decred:master May 4, 2021
@amass01 amass01 deleted the rpcclientserver branch May 6, 2021 15:20
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.

use decred's rpcclient for BTC with all requests going through RawRequest
4 participants