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

Can I help? #6

Open
dpc opened this issue Oct 23, 2018 · 8 comments
Open

Can I help? #6

dpc opened this issue Oct 23, 2018 · 8 comments

Comments

@dpc
Copy link
Contributor

dpc commented Oct 23, 2018

Hi,

I have some non-prod side project at dayjob, that was using my own hand-rolled library like this one. I'd like to port it over to this code. No commitments (as I said, it's a side-project), but I think I can push this project forward a bit here and there.

I will use this thread to post some questions etc. If you prefer eg. gitter channel, I'm happy to use that instead.

Is this library supposed to be 1:1 Bitcoin-RPC, or can it include some glue logic? Eg.

I need getblock. getblock Bitcoin RPC comes in two version. Non-verbose returns hex-encoded data, verbose big Json breakdown of TX. How should the API for this call look? fn getblock -> String and fn getblock_verbose -> Block? Is it reasonable to have another API call, that does non-verbose getblock and then hex-decode and consensus-decode to return bitcoin::block::Block directly?

@dpc
Copy link
Contributor Author

dpc commented Oct 23, 2018

Another question/suggestion: In my library I've renamed APIs to be Rust-like. Eg. get_block. Good/bad idea?

@dpc
Copy link
Contributor Author

dpc commented Oct 23, 2018

Are you OK with using rustfmt for everything? If so, can you run cargo fmt once and commit it as a start? And maybe add rustfmt.toml file to show that it's encouraged/required.

@dpc
Copy link
Contributor Author

dpc commented Oct 23, 2018

I don't think rpc_method is very useful. rpc_request seems OK, but Just the fact that's rpc_method "contains code" can confuse IDEs, text formatters and readers. I would rather take the little duplication instead.

BTW. In my code I had:

    pub fn do_rpc<T: for<'a> serde::de::Deserialize<'a>>(                                                                           
        &mut self,                                                                                                                  
        rpc_name: &str,                                                                                                             
        args: Vec<serde_json::value::Value>,                                                                                        
    ) -> Result<T> {                                                                                                                
        let request = self.rpc_client.build_request(rpc_name.to_string(), args);                                                    
        let response = self.rpc_client.execute(request);                                                                            
        let response = response?;                                                                                                   
                                                                                                                                    
        if let Some(err) = response.error {                                                                                         
            bail!("{:#?}", err);                                                                                                    
        }                                                                                                                           
                                                                                                                                    
        Ok(response                                                                                                                 
            .clone()                                                                                                                
            .into_result()                                                                                                          
            .with_context(|e| format!("{}; response: {:?}", e, response))?)                                                         
    }  

and then my methods looked like this:

    /// Generate new receiving address                                                                                              
    pub fn get_new_address(&mut self, account: Account) -> Result<Address> {                                                        
        self.do_rpc("getnewaddress", vec![account.into()])                                                                          
    }                                                                                                                               

I guess the rpc_request is a little better, but just an example that even that is not necessary.

@dpc
Copy link
Contributor Author

dpc commented Oct 23, 2018

rpc_request returning from inside itself (contains ?) is actually confusing.

TODO: unwraps when deserializing rpc responses is probably a bad idea.

@dpc
Copy link
Contributor Author

dpc commented Oct 23, 2018

Bitcoin Json RPC uses floats for balances (because JavaScript can't do integers?). In the APIs here, should we also use floats (lame), or should we take and return u64 and convert them on fly to floats that JSON needs?

@jeandudey
Copy link
Owner

jeandudey commented Nov 7, 2018

Hi! Thanks for this, I'm ok with much of what you have been said (especially that thing with macros, that was a poor idea on my part), I'm looking at your PRs and they seem ok to me, what order do you prefer to get them merged?

Pd: I can't release a new version on crates.io right now (I don't have my computer with me :/ )

@jeandudey jeandudey reopened this Nov 7, 2018
@jeandudey
Copy link
Owner

Or would you prefer write access in the repository?

@dpc
Copy link
Contributor Author

dpc commented Nov 7, 2018

Write access would be awesome. I think we're eventually going to create a new repo and push-there, under rust-bitcoin org, but before that I could push all my changes to master and eventually at least mention in the README that there's a new location.

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

No branches or pull requests

2 participants