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

Less cloning and additional pattern simplifications #326

Merged
merged 5 commits into from
Feb 7, 2020

Conversation

quentinlesceller
Copy link
Member

Similar to mimblewimble/grin#3223.
Shouldn't change any logic.

@@ -1344,7 +1344,7 @@ where

fn get_stored_tx(&self, tx: &TxLogEntry) -> Result<Option<TransactionV3>, ErrorKind> {
Owner::get_stored_tx(self, None, tx)
.map(|x| x.map(|y| TransactionV3::from(y)))
Copy link
Member

Choose a reason for hiding this comment

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

Lol

/// let valid = api_owner.verify_payment_proof(None, &p);
/// if let Ok(_) = valid {
/// //...
/// }
Copy link
Member

Choose a reason for hiding this comment

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

What are you running on it to tidy up doc comment formatting? Rustfmt doesn't get it

Copy link
Member Author

Choose a reason for hiding this comment

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

Did it manually...

shared_pubkey
.mul_assign(&secp, &sec_key)
.map_err(|e| ErrorKind::Secp(e))?;
.map_err(ErrorKind::Secp)?;
Copy link
Member

Choose a reason for hiding this comment

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

Does this not lose the actual contents of the error?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, wait, no it doesn't, never mind.

{
let mut s = self.shared_key.lock();
*s = Some(shared_key);
}

let pub_key =
PublicKey::from_secret_key(&secp, &sec_key).map_err(|e| ErrorKind::Secp(e))?;
let pub_key = PublicKey::from_secret_key(&secp, &sec_key).map_err(ErrorKind::Secp)?;
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -107,7 +106,7 @@ impl HttpSlateSender {

// trivial tests for now, but will be expanded later
if foreign_api_version < 2 {
let report = format!("Other wallet reports unrecognized API format.");
let report = "Other wallet reports unrecognized API format.".to_string();
Copy link
Member

Choose a reason for hiding this comment

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

Some of this is 6 of one, half a dozen of the other kind of stuff. to_string is probably marginally more efficient.

@@ -203,7 +202,7 @@ impl WalletSeed {
Run \"grin-wallet init\" to initialize a new wallet.",
seed_file_path
);
Err(ErrorKind::WalletSeedDoesntExist)?
Err(ErrorKind::WalletSeedDoesntExist.into())
Copy link
Member

Choose a reason for hiding this comment

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

Why does this thing hate the ? operator so much?

Copy link
Member Author

Choose a reason for hiding this comment

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

^^ it’s just more straightforward to return directly the error.

@yeastplume
Copy link
Member

yeastplume commented Feb 7, 2020

There's a lot to go over here on one go, most of it is fair enough, some of it looks like someone imposing their very opinionated personal beliefs on how some things should be done, but there's likely some justification behind them that I'm not interested in bikeshedding about.

@yeastplume yeastplume merged commit 1116bc5 into mimblewimble:master Feb 7, 2020
@quentinlesceller quentinlesceller deleted the fix branch February 7, 2020 12:55
@quentinlesceller
Copy link
Member Author

quentinlesceller commented Feb 7, 2020

Hehe, most of it makes sense though. And I left the match on boolean untouched 😛

antiochp pushed a commit to antiochp/grin-wallet that referenced this pull request Aug 7, 2020
* API cleanup

* Config cleanup

* Impl cleanup

* Libwallet cleanup

* Additionnal simplification
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.

2 participants