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/rpcserver: Add withdraw route. #422

Merged
merged 3 commits into from
Jun 15, 2020
Merged

Conversation

JoeGruffins
Copy link
Member

No description provided.

@JoeGruffins JoeGruffins force-pushed the withdrawl branch 3 times, most recently from cc9fc22 to b5760dd Compare June 3, 2020 10:04
@JoeGruffins JoeGruffins marked this pull request as ready for review June 3, 2020 10:08
client/rpcserver/handlers.go Outdated Show resolved Hide resolved
client/rpcserver/handlers.go Outdated Show resolved Hide resolved
client/rpcserver/handlers.go Outdated Show resolved Hide resolved
client/rpcserver/rpcserver.go Outdated Show resolved Hide resolved
client/rpcserver/handlers.go Outdated Show resolved Hide resolved
client/rpcserver/handlers.go Outdated Show resolved Hide resolved
Comment on lines 32 to 40
RPCArgumentsError // 16
RPCTradeError // 17
RPCCancelError // 18
SignatureError // 19
SerializationError // 20
TransactionUndiscovered // 21
ContractError // 22
SettlementSequenceError // 23
ResultLengthError // 24
IDMismatchError // 25
RedemptionError // 26
IDTypeError // 27
AckCountError // 28
UnknownResponseID // 29
OrderParameterError // 30
UnknownMarketError // 31
ClockRangeError // 32
FundingError // 33
CoinAuthError // 34
UnknownMarket // 35
NotSubscribedError // 36
UnauthorizedConnection // 37
AuthenticationError // 38
PubKeyParseError // 39
FeeError // 40
InvalidPreimage // 41
PreimageCommitmentMismatch // 42
UnknownMessageType // 43
AccountClosedError // 44
MarketNotRunningError // 45
RPCWithdrawError // 19
SignatureError // 20
SerializationError // 21
TransactionUndiscovered // 22
ContractError // 23
SettlementSequenceError // 24
Copy link
Member

Choose a reason for hiding this comment

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

At some point we need to look at which errors are for which subsystems (dcrdex comms vs dexcctl) and separate into separate enums.

@chappjc
Copy link
Member

chappjc commented Jun 5, 2020

@chappjc
Copy link
Member

chappjc commented Jun 5, 2020

It looks like the pingPeriod might be too fast or the pongWait too short vs pingPeriod in comms_test.go for the CI machines. 90 ms is a terribly fast ping period so I wouldn't be surprised if these pathetic CI machines just freeze for longer than that for no apparent reason. Otherwise I don't know why the conn write would have gotten connection reset by peer there.
Not related to this PR other than the checks hit it.

@JoeGruffins
Copy link
Member Author

Had I not made an issue for this one? I need to look over, will make one if I haven't.

Yeah, I don't know what to do about the sleeps. Do other repos not face this problem?

@JoeGruffins
Copy link
Member Author

documented in #450

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

LGTM now, but not tested. Also needs review from @buck54321 prior to merge.

@buck54321 buck54321 mentioned this pull request Jun 11, 2020
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.

Looking good, but see #464.

client/rpcserver/handlers.go Outdated Show resolved Hide resolved
@JoeGruffins JoeGruffins force-pushed the withdrawl branch 2 times, most recently from 93a5bf8 to 8cca9c2 Compare June 11, 2020 07:06
@chappjc
Copy link
Member

chappjc commented Jun 11, 2020

PR #464 is in, if you need to make updates. I don't see anything to change though.

@chappjc chappjc merged commit eac1b9c into decred:master Jun 15, 2020
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