Add path payment (find path) endpoint to cli #215

robertDurst · 2018-05-13T05:22:02Z

Is there a GIF that reflects how this work made you feel?

A few things to especially note, ponder, and critique:

Here I have to translate the user input into the 10 ^ 7 we use for the Amount type. Sort of weird IMO.
I do this iterator peekable thing here and then check to see if the iterator is empty here since it is possible for the user to receive 0 records in return. If this is the case I print: No results found. Please try again.
I call this cli subcommand find-path. Should this be an underscore or is a dash fine (I based this off of pub_net vs pub-net -- we use the dash for pub-net)?
Again, wrote these comments when I was pretty tired so if the comments are weak, please let me know 😅

Closes #168

correlator

Looking pretty good, let's integrate a few of the other things we've built to DRY up the PR

correlator · 2018-05-13T17:12:59Z

cli/src/find_path.rs

+            source_account,
+            destination_account,
+            destination_asset,
+            Amount::new(destination_amount.parse::<i64>().unwrap() * 10000000),


I can understand why this doesn't make you feel good. We ultimately decided that because stellar works with stroops and displays units scaled down by 10^7 to its users, we would follow the same convention. https://www.stellar.org/developers/guides/concepts/assets.html#amount-precision-and-representation .

I would recommend adding underscores to that big number where a comma would typically go i.e. 10_000_000

correlator · 2018-05-13T17:39:23Z

cli/src/main.rs

+                                    .takes_value(true)
+                                    .help("The code of the destination asset"),
+                            )
+                            .arg(


We've added a new asset_identifier struct to the CLI https://github.com/kbacha/stellar-sdk/blob/master/cli/src/asset_identifier.rs and we can now shorten the asset inputs like so https://github.com/kbacha/stellar-sdk/blob/master/cli/src/main.rs#L175-L180

Ok sweet, noted. Got so wrapped up in work and that memo PR from last week I missed some of these 🔥 changes.

correlator · 2018-05-13T17:40:54Z

cli/src/fmt/simple/payment_path.rs

+        );
+        append_to_buffer!(buf, "PATH:");
+        for asset in payment_path.path().iter() {
+            append_to_buffer!(buf, "   Path asset code:                  {}", asset.code());


Looks like you have some leading spaces here to indent the path attributes. I can't wait to have @alexanderreiff 's pr merged so you can utilize all the awesome tooling he's putting out around this.

Dude pumped here too. I'll leave this here for now.

correlator · 2018-05-13T17:44:57Z

cli/src/find_path.rs

+
+    if !&iter.peek().is_some() {
+        println!("No results found. Please try again.")
+    }


I think that printing nothing on a get that has no results is fine, but happy to defer to kevin here.

Ok, Ill go with nothing -- will allow me to get rid of this weird peekable stuff.

robertDurst · 2018-05-13T21:42:35Z

Thanks for the review Scott. I say we put this on hold until after @alexanderreiff 's PR is merged so that I can reformat/adjust my indentation for the payment paths.

@correlator one thing I noticed is, while the assetidentifier CLI is 🔥 , it requires you to do "XLM" -- it is case sensitive. Would y'all be open to matching any casing aka "xlm" or "XLM"?

I am sure the answer is probably -- yeah sure go fix it 😄 but just wanted to hear your thoughts in case you think differently.

alexanderreiff · 2018-05-15T00:04:11Z

@robertDurst the indent/nest stuff is finally merged! The macro names are now simplified slightly at @kbacha's suggestion: https://github.com/kbacha/stellar-sdk/blob/37764c033a75cb1bb1054e52f26e90daa5045d7d/cli/src/fmt/simple/mod.rs#L2-L23

robertDurst · 2018-05-15T03:29:23Z

Awesome! Updated for @alexanderreiff 's sweet formatting improvement.

alexanderreiff · 2018-05-15T03:33:42Z

cli/src/fmt/simple/payment_path.rs

+        append!(
+            buf,
+            "Destination asset code:           {}",
+            payment_path.destination_asset().code()


We have a Render implementation for AssetIdentifier now that gives the "standard" 1 liner: XLM or <code>-<issuer>. You could call self.render(payment_path.destination_asset()).unwrap() and not have to append each part individually.

Nice catch! Updated

choubacha · 2018-05-15T16:21:05Z

cli/src/fmt/simple/payment_path.rs

+        );
+        append!(
+            buf,
+            "Source asset:                     {}",


I'm not sure that we need so much space between label and value here.

choubacha · 2018-05-15T16:26:17Z

cli/src/main.rs

+                                    .help("The source account id of the payment path to query"),
+                            )
+                            .arg(
+                                Arg::with_name("destination_asset")


There's only one asset that needs to be specified. It might make sense to label the action asset instead of making it longer.

In fact, you may think about how we can make this easy to use from the command line. Long argument names are a pain to type out and so it might make sense to make them shorter (the name of the arg can stay the same):

stellar find-path --from=<source account id> --to=<destination account id> --asset=XLM --amount=1.123456

choubacha · 2018-05-15T16:28:26Z

cli/src/find_path.rs

+            source_account,
+            destination_account,
+            destination_asset,
+            Amount::new(destination_amount.parse::<i64>().unwrap() * 10_000_000),


Parsing to an i64 will lose the decimals: 1.123 will be 1, etc. We should try to capture what they mean exactly, similar to how we do it in the client library. In fact, it might make sense to even implement a FromStr trait on the amount type so that we can handle the parsing cleanly (and without fidelity loss).

Ok, so this was fun, but a little wonky -- I noted in the code w/ comments where the wonkiness comes into play.

robertDurst · 2018-05-18T03:50:43Z

@kbacha thanks for the feedback. Made the appropriate changes. Let me know if there is anything else, or if my FromStr or From trait implementations (is that the correct terminology?) can be done better.

Also note, a trade_aggregations test started to fail so I just extended the start_time parameter to encompass a trade.

choubacha · 2018-05-18T16:51:56Z

cli/src/find_path.rs

+            source_account,
+            destination_account,
+            destination_asset,
+            Amount::from_str(destination_amount).unwrap(),


I generally like to use expect here instead so that you can provide a message. Of course, we should really be returning an Result from this path and allowing it to be handled by the outer logic.

Would this be more appropriate -- to parse and throw an error as part of the argument matching process? Although I admit the double expect does not seem right.

let destination_amount: Amount = matches .value_of("amount") .expect("Destination amount is a required field") .parse() .expect("Amount must be properly formatted");

This would then allow:

let endpoint = payment::FindPath::new( source_account, destination_account, destination_asset, destination_amount, );

Yup, that would be better I think. I've been recommending just returning an Error so that exit is centralized, but it's a CLI so it's not the end of the world.

Eventually, centralized error reporting, would provide a consistently formatted error text though.

choubacha · 2018-05-18T16:53:05Z

client/src/endpoint/payment.rs

+        let params = wrap.params();
+        let destination_asset = AssetIdentifier::new(
+            params.get_ok("destination_asset_type")?,
+            params.get_parse("destination_asset_code").ok(),


These are just strings right? Do you need to get parse them?

Yes, true. Maybe I am missing something, but when I do:

params.get("destination_asset_code")

I get the following error:

note: expected type `std::option::Option<std::string::String>` found type `std::option::Option<&str>`

Ah, it wants it to be owned, so you are actually parsing it. Carry on, nothing to see here.

choubacha · 2018-05-18T16:53:51Z

client/src/endpoint/payment.rs

+            source_account: params.get_parse("source_account")?,
+            destination_account: params.get_parse("destination_account")?,
+            destination_asset: destination_asset,
+            destination_amount: Amount::new(params.get_parse("destination_account")?),


Technically if Amount implements the FromStr trait, you should be able to parse to it.

Cool, makes sense. However (noob question alert 😄) I get this error:

the trait std::convert::From<resources::amount::ParseIntError> is not implemented for uri::Error

meaning the two error types are different. Here and in general, how do I deal with situations where a functions expected error type and an expressions error type differ?

So, that would mean that we are missing a conversion for our error type for amount into the result error type.

Also, I just noticed that you are parsing the account into an amount. That can't be right.

🤔 that is a valid point

choubacha · 2018-05-18T16:54:39Z

client/src/resources/amount.rs

+/// Recreating ParseIntError since its interior is private
+/// https://doc.rust-lang.org/src/core/num/mod.rs.html#3949
+#[derive(Debug)]
+pub enum ParseIntError {


Better would be to use ParseAmountError since that's what we are parsing.

choubacha · 2018-05-18T16:57:35Z

client/src/resources/amount.rs

+impl From<num::ParseIntError> for ParseIntError {
+    fn from(error: num::ParseIntError) -> Self {
+        match error.to_string().as_ref() {
+            "cannot parse integer from empty string" => ParseIntError::Empty,


These may change with different versions of rust so I wouldn't rely on them for matching if possible. If you want to convert I would actually just wrap the ParseIntError:

pub enum ParseAmountError { IntError(ParseIntError), }

Then the from implementation is easier:

impl From<num::ParseIntError> for ParseAmountError { fn from(error: num::ParseIntError) -> ParseAmountError { ParseAmountError::IntError(error) } }

If desired, you can implement Error trait on this if you want to output the inner description. But It should capture the data you need.

Oh yes, thanks good point. I guess the reason I created the entire matching was because I was unable to figure out how to properly throw an error here:

if number_decimals > 7 { Err(ParseIntError::Underflow) }

The reason is that the ParseIntError's value is private so I couldn't instantiate/create a new ParseIntError. Any recommendations here?

Ok, sure, let's start by thinking about what Underflow means. That's when we exceed an interger bit-size and end up at the lowest value. That's not actually what this condition means, it's what the condition is preventing. The condition is saying that we can never have more than 7 decimal places.

As such, it would be good to have that as a specific Amount error:

pub enum ParseAmountError { ExceedsDecimalLength, IntError(num::ParseIntError), }

choubacha · 2018-05-18T17:00:20Z

client/src/resources/amount.rs

+    type Err = ParseIntError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s.rfind('.') {


This is copy-pasta from the deserialize impl, I think. It would be better to switch to using this implementation to implement the Deserialize function and map to the correct errors that it expects there.

Yes, I admit I copy-pasta this. To clarify, are you saying I should keep FromStr and then utilize the FromStr implementation as part of deserialize, removing the code duplication, then mapping the errors of FromStr to appropriately match the Deserialize error type?

Yup, use FromStr within the deserialize and map the "specific" type errors to a deserialize custom error.

choubacha · 2018-05-18T17:00:48Z

client/src/resources/amount.rs

+
+    #[test]
+    fn it_raises_amount_by_ten_million() {
+        let amount: Amount = Amount::from_str("2.12").unwrap();


another way:

let amount: Amount = "2.12".parse().unwrap();

robertDurst · 2018-05-20T15:58:40Z

@kbacha thanks for the review and thanks for your patience! I am struggling more so with this PR than I thought I would -- although I am having a pretty good time working with traits and what not.

robertDurst · 2018-05-22T05:13:27Z

@kbacha so I made all the above edits except I still don't use the from_str trait for:

destination_amount: Amount::new(params.get_parse("destination_account")?)

Unless I am mistaken, I could use params.get which would return a &str. However from_str wants a String, so it may be cleaner to just use get_parse?

Must say the deserialize looks pretty awesome:

impl<'de> Deserialize<'de> for Amount {
    fn deserialize<D>(d: D) -> Result<Amount, D::Error>
    where
        D: Deserializer<'de>,
    {
        let s = String::deserialize(d)?;
        let amount: Amount = Amount::from_str(&s).map_err(|e| match e {
            ParseAmountError::ExceedsDecimalLength => {
                de::Error::custom("Amount has too many digits of precision.")
            }
            ParseAmountError::IntError(err) => de::Error::custom(err.to_string()),
        })?;
        Ok(amount)
    }
}

Map error with a match is pretty nifty -- assuming this is correct use of map_err + match 😄

Once this has the green light I will squash

robertDurst · 2018-05-27T23:50:47Z

This latest commit may have gone a bit deep down the rabbit hole 😅

Essentially, I needed to implement TryFromUri for the find path endpoint. Here is what I got to work, which is pretty nice:

impl TryFromUri for FindPath {
    fn try_from_wrap(wrap: &UriWrap) -> ::std::result::Result<FindPath, uri::Error> {
        let params = wrap.params();
        Ok(FindPath {
            source_account: params.get_parse("from")?,
            destination_account: params.get_parse("to")?,
            destination_asset: params.get_parse("asset")?,
            destination_amount: params.get_parse("amount")?,
        })
    }
}

To do this however, I had to do a few things:

add ParseAmountError and ParseAssetError as enums on the Uri::Error
ported the asset_identifier from_str to Asset Resource

I think this all logically is sound, however I am not 100% sure this is the best approach.

choubacha

It looks like you are using a lot of strings for errors, please avoid that.

choubacha · 2018-05-28T15:27:06Z

client/src/resources/amount.rs

+pub enum ParseAmountError {
+    /// An error describing the case where the amount contains
+    /// too many digits of precision to parse correctly
+    ExceedsDecimalLength(String),


I don't think we need a string inside this type. I think the type explains what's wrong well enough.

choubacha · 2018-05-28T15:27:31Z

client/src/resources/amount.rs

@@ -141,23 +142,82 @@ impl<'de> Deserialize<'de> for Amount {
        D: Deserializer<'de>,
    {
        let s = String::deserialize(d)?;
+        let amount: Amount = Amount::from_str(&s).map_err(|e| match e {
+            ParseAmountError::ExceedsDecimalLength(_val) => {


Hopefully we can remove the string inner value since it's ignored anyhow.

choubacha · 2018-05-28T15:29:34Z

client/src/resources/asset.rs

+/// you get an error.
+#[derive(Debug)]
+pub struct ParseAssetError {
+    details: String,


Strings make for bad error typing. You should use an Enum with the possible errors coded into it. Especially as there is only one error possible.

choubacha · 2018-05-28T15:30:22Z

client/src/resources/asset.rs

+    }
+}
+
+impl FromStr for AssetIdentifier {


I see we are adding this trait impl here but I don't see it removed from the CLI, is there a reason for that?

Will hold off for a future PR -- I should probably create a new issue

choubacha · 2018-05-28T15:30:57Z

client/src/resources/asset.rs

+/// When a bad token or string is provided to parsing into an asset
+/// you get an error.
+#[derive(Debug)]
+pub struct ParseAssetError {


This error type is confusing. We are parsing the asset identifier, not an asset itself, right? If so, this is named incorrectly.

correlator · 2018-06-03T19:08:57Z

client/src/endpoint/trade.rs

@@ -359,7 +359,7 @@ mod all_trades_tests {
 ///
 /// // Place the start time in the past so we capture it.
 /// let agg = trade::Aggregations::new(base, counter)
-///     .with_start_time(now - 3_000_000_000)
+///     .with_start_time(now - 50_000_000_000)


This is always going to be falling out of style over time. Should we just start at 0?

choubacha · 2018-06-07T16:44:19Z

client/src/resources/operation/test.rs

-                        "type":"{}",
-                        "transaction_hash":"123"
-                    }}"#,
+                                    "id":"1",


Looks like you are on an old version of rust now, try updating so this doesn't change.

oh man... let me check.

@kbacha just ran cargo fmt --all w/ 1.26.2 installed and nothing changed. However, moved it back and ran fmt and still nothing changed 🤔 . Anyway just reformatted and pushed.

robertDurst · 2018-06-07T23:52:07Z

@kbacha thoughts?

choubacha · 2018-06-11T17:42:05Z

cli/src/main.rs

+        .subcommand(
+            SubCommand::with_name("find-path")
+                 .about("Fetch possible payment paths")
+                            .arg(


whoa, why is this indentation odd?

lol again, not caught (and at the same time not fixed) by cargo fmt

choubacha · 2018-06-11T19:08:15Z

client/src/resources/amount.rs

+    }
+}
+
+/// Recreating ParseIntError since its interior is private


comment should be changed to reflect changes.

robertDurst · 2018-06-11T19:37:11Z

@kbacha changes made!

robertDurst · 2018-06-11T19:37:28Z

Once this is good to go, will rebase + squash

choubacha

I'm good with the changes, squash down the commits and it's ready to merge!!

robertDurst · 2018-06-11T20:06:51Z

Rebased and Squashed.

Need a new:

Is there a GIF that reflects how this work made you feel?

robertDurst requested review from choubacha and correlator May 13, 2018 05:22

correlator reviewed May 13, 2018

View reviewed changes

alexanderreiff reviewed May 15, 2018

View reviewed changes

choubacha reviewed May 15, 2018

View reviewed changes

choubacha reviewed May 18, 2018

View reviewed changes

choubacha reviewed May 28, 2018

View reviewed changes

correlator reviewed Jun 3, 2018

View reviewed changes

This was referenced Jun 6, 2018

Determine how we should handle streaming end points #52

Open

Remove FromStr for AssetIdentifier from CLI #223

Closed

choubacha reviewed Jun 7, 2018

View reviewed changes

choubacha approved these changes Jun 11, 2018

View reviewed changes

Add path payment (find path) endpoint to cli

9697b6a

choubacha merged commit b40f7af into choubacha:master Jun 11, 2018

robertDurst mentioned this pull request Jun 17, 2018

Remove FromStr for AssetIdentifier from CLI #224

Merged

Add path payment (find path) endpoint to cli #215

Add path payment (find path) endpoint to cli #215

Conversation

robertDurst commented May 13, 2018 • edited Loading

Is there a GIF that reflects how this work made you feel?

correlator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented May 13, 2018 • edited Loading

alexanderreiff commented May 15, 2018

robertDurst commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented May 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst May 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented May 20, 2018

robertDurst commented May 22, 2018 • edited Loading

robertDurst commented May 27, 2018

choubacha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertDurst commented Jun 11, 2018

robertDurst commented Jun 11, 2018

choubacha left a comment

Choose a reason for hiding this comment

robertDurst commented Jun 11, 2018

Is there a GIF that reflects how this work made you feel?

robertDurst commented May 13, 2018 •

edited

Loading

robertDurst commented May 13, 2018 •

edited

Loading

robertDurst May 20, 2018 •

edited

Loading

robertDurst commented May 22, 2018 •

edited

Loading