LBRYCommunity/lbry.go
1
0
Fork 0
Code Issues 5 Pull requests Projects Releases 46 Packages Wiki Activity

Add wallet commands to JSON-RPC client #73

Merged
anbsky merged 8 commits from feature/wallet into master 2019-10-08 08:08:57 +02:00
Conversation 2 Commits 8 Files changed 5 +245 -30
anbsky commented 2019-09-25 12:47:55 +02:00 (Migrated from github.com)
Copy link

This adds new wallet_* commands.

This adds [new wallet_* commands](https://lbry.tech/api/sdk#wallet_add).
tiger5226 (Migrated from github.com) reviewed 2019-09-25 12:47:55 +02:00
nikooo777 (Migrated from github.com) reviewed 2019-09-25 18:11:11 +02:00
nikooo777 (Migrated from github.com) left a comment
Copy link

refer to the comments for improvements

refer to the comments for improvements
extras/jsonrpc/daemon.go
@ -142,3 +143,4 @@
return response, d.call(response, "account_list", map[string]interface{}{})
}
func (d *Client) AccountListForWallet(walletID string) (*AccountListResponse, error) {
nikooo777 (Migrated from github.com) commented 2019-09-25 16:18:04 +02:00
Copy link

i think we should just add an optional param to the current account functions. just like I did for AccountAdd yesterday

i think we should just add an optional param to the current account functions. just like I did for AccountAdd yesterday
extras/jsonrpc/daemon.go
nikooo777 (Migrated from github.com) commented 2019-09-25 18:09:37 +02:00
Copy link

you gotta remember to define the json part and add ,omitempty otherwise the SDK will crap out

you gotta remember to define the `json` part and add `,omitempty` otherwise the SDK will crap out
extras/jsonrpc/daemon.go
nikooo777 (Migrated from github.com) commented 2019-09-25 16:19:22 +02:00
Copy link

any reasons as to why you'd want to use a structure instead of just passing 3 more params?
to stay consistent with the other calls, unless it's a lot of params we're talking about, I would keep it simple

any reasons as to why you'd want to use a structure instead of just passing 3 more params? to stay consistent with the other calls, unless it's a lot of params we're talking about, I would keep it simple
nikooo777 (Migrated from github.com) commented 2019-09-25 18:08:50 +02:00
Copy link

we followed up in slack. I think we need more opinions here.
Struct pros:

  • easier to manage when not all params are necessary so that we don't have to specify nil each time
d.WalletCreate(id, nil)
vs
d.WalletCreate(id, nil, nil, nil, nil)

Cons:

  • When the structure changes, those using the library will not necessarily notice and the behavior of the function might not reflect the intended purpose making it unclear as to what needs to be changed (tests should prevent that)
we followed up in slack. I think we need more opinions here. Struct pros: - easier to manage when not all params are necessary so that we don't have to specify nil each time ``` d.WalletCreate(id, nil) vs d.WalletCreate(id, nil, nil, nil, nil) ``` Cons: * When the structure changes, those using the library will not necessarily notice and the behavior of the function might not reflect the intended purpose making it unclear as to what needs to be changed (tests should prevent that)
anbsky (Migrated from github.com) reviewed 2019-09-25 18:44:58 +02:00
extras/jsonrpc/daemon.go
@ -142,3 +143,4 @@
return response, d.call(response, "account_list", map[string]interface{}{})
}
func (d *Client) AccountListForWallet(walletID string) (*AccountListResponse, error) {
anbsky (Migrated from github.com) commented 2019-09-25 18:44:58 +02:00
Copy link

That's how we ended up with v2 of lbry.go already — introducing backwards incompatible changes way too liberally. I don't think we should do that.

P.S. That's another example where opts would make things easier by not breaking backwards compatibility.

That's how we ended up with v2 of lbry.go already — introducing backwards incompatible changes way too liberally. I don't think we should do that. P.S. That's another example where opts would make things easier by not breaking backwards compatibility.
anbsky (Migrated from github.com) reviewed 2019-09-25 18:51:26 +02:00
extras/jsonrpc/daemon.go
anbsky (Migrated from github.com) commented 2019-09-25 18:51:25 +02:00
Copy link

It also makes it easier to omit optional parameters and supply only the ones we want. The second example is much more readable in my opinion.

d.AccountAdd("account_name", nil, nil, nil, true, nil)
// vs 
d.AccountAdd("account_name", &AccountOpts{SingleKey: true})

This also makes sense where we're designing an RPC client, which, in turn, relies on an API that is changing very rapidly. We don't want to bump major version number and/or break, say, AccountList for all clients just because there is a new WalletID parameter now that we need to use.

If the structure changes in a non-backwards compatible way, it should be fairly easy to validate in the receiving functions and produce an error at run time.

Bottom line, I think losing some of the compile time validation here is worth it for the ease of readability and use.

It also makes it easier to omit optional parameters and supply only the ones we want. The second example is much more readable in my opinion. ``` d.AccountAdd("account_name", nil, nil, nil, true, nil) // vs d.AccountAdd("account_name", &AccountOpts{SingleKey: true}) ``` This also makes sense where we're designing an RPC client, which, in turn, relies on an API that is changing very rapidly. We don't want to bump major version number and/or break, say, `AccountList` for all clients just because there is a new `WalletID` parameter now that we need to use. If the structure changes in a non-backwards compatible way, it should be fairly easy to validate in the receiving functions and produce an error at run time. Bottom line, I think losing some of the compile time validation here is worth it for the ease of readability and use.
lyoshenka (Migrated from github.com) reviewed 2019-09-27 16:41:11 +02:00
extras/jsonrpc/daemon.go
lyoshenka (Migrated from github.com) commented 2019-09-27 16:41:10 +02:00
Copy link

Reposting from slack:

to me it makes sense to use arguments for required params and an options struct for optional params

alternatively, if there's an option that's used a lot when calling the method, that can be an arg too

Reposting from slack: to me it makes sense to use arguments for required params and an options struct for optional params alternatively, if there's an option that's used a lot when calling the method, that can be an arg too
lyoshenka (Migrated from github.com) reviewed 2019-09-27 16:43:00 +02:00
extras/jsonrpc/daemon.go
lyoshenka (Migrated from github.com) commented 2019-09-27 16:42:59 +02:00
Copy link

@sayplastic I'm confused what you mean by

and break, say, AccountList for all clients just because there is a new WalletID parameter now that we need to use

How would we break AccountList for all clients? it would not break unless they update to a newer version of lbry.go. And if they are updating, they should be aware that there may be API changes that they'll have to handle. Nothing will break without them taking action.

@sayplastic I'm confused what you mean by > and break, say, AccountList for all clients just because there is a new WalletID parameter now that we need to use How would we break AccountList for all clients? it would not break unless they update to a newer version of lbry.go. And if they are updating, they should be aware that there may be API changes that they'll have to handle. Nothing will break without them taking action.
anbsky (Migrated from github.com) reviewed 2019-09-28 11:04:26 +02:00
extras/jsonrpc/daemon.go
anbsky (Migrated from github.com) commented 2019-09-28 11:04:25 +02:00
Copy link

Not the best wording on my part. What I meant is we’re going to be requiring changes from client code if they stay up to date.

Not the best wording on my part. What I meant is we’re going to be requiring changes from client code if they stay up to date.
nikooo777 (Migrated from github.com) reviewed 2019-09-30 16:04:41 +02:00
extras/jsonrpc/daemon.go
@ -142,3 +143,4 @@
return response, d.call(response, "account_list", map[string]interface{}{})
}
func (d *Client) AccountListForWallet(walletID string) (*AccountListResponse, error) {
nikooo777 (Migrated from github.com) commented 2019-09-30 16:04:40 +02:00
Copy link

I think we shouldn't worry too much about backward compatibility. If one doesn't want to stay up to date with lbry.go/sdk then they can lock themselves into a commit that supports such old versions.
We don't have the time to support multiple versions, only the current one. I still feel like it should not be split out and rather updated

I think we shouldn't worry too much about backward compatibility. If one doesn't want to stay up to date with lbry.go/sdk then they can lock themselves into a commit that supports such old versions. We don't have the time to support multiple versions, only the current one. I still feel like it should not be split out and rather updated
nikooo777 (Migrated from github.com) reviewed 2019-09-30 16:07:13 +02:00
extras/jsonrpc/daemon.go
nikooo777 (Migrated from github.com) commented 2019-09-30 16:07:13 +02:00
Copy link

that's the desired outcome in my opinion. If one client updates lbry.go they should expect to update the use of their functions according to the SDK changes. Hiding those updates will fragment usage and possibly make updating a pain as one should either have all sdk functionalities tested (which would be great, but realistically speaking unlikely) or go through all the calls to figure out the changes

that's the desired outcome in my opinion. If one client updates lbry.go they should expect to update the use of their functions according to the SDK changes. Hiding those updates will fragment usage and possibly make updating a pain as one should either have all sdk functionalities tested (which would be great, but realistically speaking unlikely) or go through all the calls to figure out the changes
anbsky (Migrated from github.com) reviewed 2019-10-02 09:22:45 +02:00
extras/jsonrpc/daemon.go
anbsky (Migrated from github.com) commented 2019-10-02 09:22:45 +02:00
Copy link

I changed the way options are provided, have a look please

I changed the way options are provided, have a look please
anbsky commented 2019-10-02 09:48:14 +02:00 (Migrated from github.com)
Copy link

I have restored versioning, after this is merged you can tag new versions as v2.3.1 v2.3.2 and higher and import them in client code as github.com/lbryio/lbry.go/v2/...

Would strongly suggest tagging your code that is not finalized yet with a pre suffix.

I have restored versioning, after this is merged you can tag new versions as ~v2.3.1~ v2.3.2 and higher and import them in client code as `github.com/lbryio/lbry.go/v2/...` Would strongly suggest tagging your code that is not finalized yet with a `pre` suffix.
anbsky commented 2019-10-03 15:06:41 +02:00 (Migrated from github.com)
Copy link

Added ChannelImport method and changed ChannelList signature for wallet ID.

Added `ChannelImport` method and changed `ChannelList` signature for wallet ID.
nikooo777 (Migrated from github.com) reviewed 2019-10-03 18:24:17 +02:00
go.mod
@ -27,4 +19,3 @@
github.com/nlopes/slack v0.5.0
github.com/pkg/errors v0.8.1 // indirect
github.com/sebdah/goldie v0.0.0-20180424091453-8784dd1ab561
github.com/sergi/go-diff v1.0.0
nikooo777 (Migrated from github.com) commented 2019-10-03 18:22:49 +02:00
Copy link

something must be still using the wrong stuff

something must be still using the wrong stuff
go.sum
@ -66,6 +66,10 @@ github.com/kkdai/bstream v0.0.0-20161212061736-f391b8402d23 h1:FOOIBWrEkLgmlgGfM
github.com/kkdai/bstream v0.0.0-20161212061736-f391b8402d23/go.mod h1:J+Gs4SYgM6CZQHDETBtE9HaSEkGmuNXF86RwHhHUvq4=
github.com/konsorten/go-windows-terminal-sequences v1.0.1 h1:mweAR1A6xJ3oS2pRaGiHgQ4OO8tzTaLawm8vnODuwDk=
github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ=
github.com/lbryio/errors.go v0.0.0-20180223142025-ad03d3cc6a5c/go.mod h1:muH7wpUqE8hRA3OrYYosw9+Sl681BF9cwcjzE+OCNK8=
nikooo777 (Migrated from github.com) commented 2019-10-03 18:23:38 +02:00
Copy link

nope, we have to use lbry.go for this I think

nope, we have to use lbry.go for this I think
nikooo777 (Migrated from github.com) requested changes 2019-10-03 18:29:24 +02:00
nikooo777 (Migrated from github.com) left a comment
Copy link

needs fixes

needs fixes
extras/jsonrpc/daemon.go
nikooo777 (Migrated from github.com) commented 2019-10-03 18:27:27 +02:00
Copy link

this needs to be converted to a defined structure like in some other calls, otherwise I believe the SDK might crap out if you pass --wallet_id=null

this needs to be converted to a defined structure like in some other calls, otherwise I believe the SDK might crap out if you pass --wallet_id=null
extras/jsonrpc/daemon.go
nikooo777 (Migrated from github.com) commented 2019-10-03 18:27:54 +02:00
Copy link

same here

same here
extras/jsonrpc/daemon_test.go
@ -510,0 +660,4 @@
_, err = d.WalletRemove(id)
if err != nil {
t.Fatal(err)
nikooo777 (Migrated from github.com) commented 2019-10-03 18:28:53 +02:00
Copy link

you should try calling an extra one without walletID to see if it holds up or not

you should try calling an extra one without walletID to see if it holds up or not
anbsky (Migrated from github.com) reviewed 2019-10-03 19:35:39 +02:00
go.sum
@ -66,6 +66,10 @@ github.com/kkdai/bstream v0.0.0-20161212061736-f391b8402d23 h1:FOOIBWrEkLgmlgGfM
github.com/kkdai/bstream v0.0.0-20161212061736-f391b8402d23/go.mod h1:J+Gs4SYgM6CZQHDETBtE9HaSEkGmuNXF86RwHhHUvq4=
github.com/konsorten/go-windows-terminal-sequences v1.0.1 h1:mweAR1A6xJ3oS2pRaGiHgQ4OO8tzTaLawm8vnODuwDk=
github.com/konsorten/go-windows-terminal-sequences v1.0.1/go.mod h1:T0+1ngSBFLxvqU3pZ+m/2kptfBszLMUkC4ZK/EgS/cQ=
github.com/lbryio/errors.go v0.0.0-20180223142025-ad03d3cc6a5c/go.mod h1:muH7wpUqE8hRA3OrYYosw9+Sl681BF9cwcjzE+OCNK8=
anbsky (Migrated from github.com) commented 2019-10-03 19:35:39 +02:00
Copy link

Strange since this is not a manual edit

Strange since this is not a manual edit
anbsky (Migrated from github.com) reviewed 2019-10-03 19:52:33 +02:00
extras/jsonrpc/daemon.go
anbsky (Migrated from github.com) commented 2019-10-03 19:52:33 +02:00
Copy link

I covered it with a test and seems like the SDK handles it well

I covered it with a test and seems like the SDK handles it well
anbsky (Migrated from github.com) reviewed 2019-10-03 20:28:32 +02:00
extras/jsonrpc/daemon.go
@ -142,3 +143,4 @@
return response, d.call(response, "account_list", map[string]interface{}{})
}
func (d *Client) AccountListForWallet(walletID string) (*AccountListResponse, error) {
anbsky (Migrated from github.com) commented 2019-10-03 20:28:31 +02:00
Copy link

I spent a few minutes adding a nil parameter to AccoutList in calls around the test code that don't know about wallet_id and it felt quite ugly to me. Barring all the uncertainties of designing an API ahead of client code, I'd hazard a guess the majority of clients will be requiring the argument-less version of account_list and we should keep it that.

I spent a few minutes adding a `nil` parameter to `AccoutList` in calls around the test code that don't know about `wallet_id` and it felt quite ugly to me. Barring all the uncertainties of designing an API ahead of client code, I'd hazard a guess the majority of clients will be requiring the argument-less version of `account_list` and we should keep it that.
Sign in to join this conversation.
Reviewers
No reviewers
tiger5226
nikooo777
Labels
Clear labels   
area: devops

   
area: discovery

   
area: docs

   
area: livestream

   
area: proposal

Work is part of a proposal

  
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
dependencies

Pull requests that update a dependency file

  
Epic

   
good first issue

   
hacktoberfest

Welcome to Hacktoberfest

  
help wanted

   
icebox

Long-term storage

  
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
on hold

Temporarily paused

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
resilience

   
Tom's Wishlist

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
dependencies
Epic
good first issue
hacktoberfest
help wanted
icebox
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry.go#73
No description provided.
Delete branch "feature/wallet"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?