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

Fixes #1960: z_getoperationstatus/result now includes operation details. #1991

Merged
merged 1 commit into from Jan 18, 2017

Conversation

Projects
None yet
5 participants
@bitcartel
Contributor

bitcartel commented Jan 7, 2017

No description provided.

@bitcartel bitcartel added this to the 1.0.5 milestone Jan 7, 2017

@bitcartel bitcartel self-assigned this Jan 7, 2017

@bitcartel bitcartel requested a review from nathan-at-least Jan 7, 2017

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Jan 7, 2017

Contributor

Output will look like this:

./zcash-cli z_getoperationstatus '["opid-890f04ff-e506-4ae9-9f2c-d79f36e19590"]'
[
    {
        "id" : "opid-890f04ff-e506-4ae9-9f2c-d79f36e19590",
        "status" : "success",
        "creation_time" : 1483771560,
        "result" : {
            "txid" : "2188be1d8e79475d3e791fe140967a780199d98bc1130c05c454cd4dfbd874f9"
        },
        "execution_secs" : 137.00294520,
        "params" : {
            "fromaddress" : "ztr4Ef2m7CFTtTvs4UpDz8zJY4Swukr3NRKThfLE12sNdGdav7Yf55G9HMAzGM3baR1FD43u9jb5JsAN67BBvz1UsVdLxoi",
            "amounts" : [
                {
                    "address" : "ztiyGQnkMJy37XCpYJf4UinTcpVaDsqJNZXQZhtQ48SJJ289EnUgN7JkCmaCQ2e16DsV9PE71iwQ7hzhCi4wPCC3m5VHPWe",
                    "amount" : 0.00100000
                },
                {
                    "address" : "ztXsDXFm6GxxzuD5JLALwezeE15jkRyNwaAniLuZuyuVUpwCcSiCHkBRbxcn5dncjwnEvbwkD2R7aEeiFJuaRvBm7SAip8q",
                    "amount" : 0.00300000
                },
                {
                    "address" : "ztZ7rUtQgW9obthFGoFuQzwHQjDTrkVBKZ5uBtR2DB1obb3DhjZsm81ndwpucFvmmK62ydFJaiofCx4cNA8vwoRc3ACuEZG",
                    "amount" : 0.00400000
                },
                {
                    "address" : "ztPVovcCjjVv4qohieJ7eUXgeBHinSHoiZ6J6os3DjCFeg1wHF11bHZ83ziRjb8zeSn1dXahTUNB6mrsoFC32nVaMvXjqFU",
                    "amount" : 0.00500000,
                    "memo" : "DEADBEEF"
                }
            ],
            "minconf" : 1,
            "fee" : 0.00010000
        }
    }
]

Contributor

bitcartel commented Jan 7, 2017

Output will look like this:

./zcash-cli z_getoperationstatus '["opid-890f04ff-e506-4ae9-9f2c-d79f36e19590"]'
[
    {
        "id" : "opid-890f04ff-e506-4ae9-9f2c-d79f36e19590",
        "status" : "success",
        "creation_time" : 1483771560,
        "result" : {
            "txid" : "2188be1d8e79475d3e791fe140967a780199d98bc1130c05c454cd4dfbd874f9"
        },
        "execution_secs" : 137.00294520,
        "params" : {
            "fromaddress" : "ztr4Ef2m7CFTtTvs4UpDz8zJY4Swukr3NRKThfLE12sNdGdav7Yf55G9HMAzGM3baR1FD43u9jb5JsAN67BBvz1UsVdLxoi",
            "amounts" : [
                {
                    "address" : "ztiyGQnkMJy37XCpYJf4UinTcpVaDsqJNZXQZhtQ48SJJ289EnUgN7JkCmaCQ2e16DsV9PE71iwQ7hzhCi4wPCC3m5VHPWe",
                    "amount" : 0.00100000
                },
                {
                    "address" : "ztXsDXFm6GxxzuD5JLALwezeE15jkRyNwaAniLuZuyuVUpwCcSiCHkBRbxcn5dncjwnEvbwkD2R7aEeiFJuaRvBm7SAip8q",
                    "amount" : 0.00300000
                },
                {
                    "address" : "ztZ7rUtQgW9obthFGoFuQzwHQjDTrkVBKZ5uBtR2DB1obb3DhjZsm81ndwpucFvmmK62ydFJaiofCx4cNA8vwoRc3ACuEZG",
                    "amount" : 0.00400000
                },
                {
                    "address" : "ztPVovcCjjVv4qohieJ7eUXgeBHinSHoiZ6J6os3DjCFeg1wHF11bHZ83ziRjb8zeSn1dXahTUNB6mrsoFC32nVaMvXjqFU",
                    "amount" : 0.00500000,
                    "memo" : "DEADBEEF"
                }
            ],
            "minconf" : 1,
            "fee" : 0.00010000
        }
    }
]

@tromer

This comment has been minimized.

Show comment
Hide comment
@tromer

tromer Jan 7, 2017

Contributor

In the future we may have additional kinds of async RPC, with different parameters, so the z_getoperationstatus output should say which async RPC it's describing.

In particular, this format follows the current z_sendmany restriction of a single source address, which may be generalized in the future (see #1785, #1811, #1555).

There are cases where z_sendmany may need to generate multiple transactions, not merely multiple JoinSplits in a single transaction, to prevent information leakage (see #1971, #1360 (comment)). The output format should generalize to that case. ← incorrect because of #1971 (comment)

Consider adding additional low-level information, e.g., about consumed notes.

Contributor

tromer commented Jan 7, 2017

In the future we may have additional kinds of async RPC, with different parameters, so the z_getoperationstatus output should say which async RPC it's describing.

In particular, this format follows the current z_sendmany restriction of a single source address, which may be generalized in the future (see #1785, #1811, #1555).

There are cases where z_sendmany may need to generate multiple transactions, not merely multiple JoinSplits in a single transaction, to prevent information leakage (see #1971, #1360 (comment)). The output format should generalize to that case. ← incorrect because of #1971 (comment)

Consider adding additional low-level information, e.g., about consumed notes.

@tromer

This comment has been minimized.

Show comment
Hide comment
@tromer

tromer Jan 13, 2017

Contributor

Please disregard what I said in the 3rd paragraph above (about z_sendmany needing to generate multiple transactions). That's incorrect because of #1971 (comment) and I now marked it as such.

Contributor

tromer commented Jan 13, 2017

Please disregard what I said in the 3rd paragraph above (about z_sendmany needing to generate multiple transactions). That's incorrect because of #1971 (comment) and I now marked it as such.

@bitcartel bitcartel requested review from daira and ebfull and removed request for daira Jan 17, 2017

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Jan 17, 2017

Contributor

@tromer Added key-value pair "method" : "z_sendmany" to the output. Rebased commit.

Information about consumed notes could be something for 1.0.6+. We'll need to consider privacy implications and the rest of the RPC API for low-level note management (sketched out prior to launch, but we focused on implementing on payment API)

Contributor

bitcartel commented Jan 17, 2017

@tromer Added key-value pair "method" : "z_sendmany" to the output. Rebased commit.

Information about consumed notes could be something for 1.0.6+. We'll need to consider privacy implications and the rest of the RPC API for low-level note management (sketched out prior to launch, but we focused on implementing on payment API)

@ebfull

This comment has been minimized.

Show comment
Hide comment
@ebfull

ebfull Jan 17, 2017

Contributor

This looks good to me!

@zkbot r+

Contributor

ebfull commented Jan 17, 2017

This looks good to me!

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Jan 17, 2017

Contributor

📌 Commit 8aa7937 has been approved by ebfull

Contributor

zkbot commented Jan 17, 2017

📌 Commit 8aa7937 has been approved by ebfull

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Jan 17, 2017

Contributor

⌛️ Testing commit 8aa7937 with merge 4169cdd...

Contributor

zkbot commented Jan 17, 2017

⌛️ Testing commit 8aa7937 with merge 4169cdd...

zkbot added a commit that referenced this pull request Jan 17, 2017

Auto merge of #1991 - bitcartel:1960_z_getoperation_include_call_deta…
…ils, r=ebfull

Fixes #1960: z_getoperationstatus/result now includes operation details.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Jan 18, 2017

Contributor

☀️ Test successful - zcash

Contributor

zkbot commented Jan 18, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit 8aa7937 into zcash:master Jan 18, 2017

1 check passed

homu Test successful
Details
@nathan-at-least

This comment has been minimized.

Show comment
Hide comment
@nathan-at-least

nathan-at-least Jan 18, 2017

Contributor

Starting review now.

Contributor

nathan-at-least commented Jan 18, 2017

Starting review now.

@nathan-at-least

Requests for changes mainly to codify the method as a first-class required part of the RPC interface in documentation and tests.

Non-blocking comment on API design alternatives.

@@ -89,6 +89,10 @@ Asynchronous calls return an OperationStatus object which is a JSON object with
* code : number
* message: error message
Depending on the type of asynchronous call, there may be other key-value pairs. For example, a z_sendmany operation will also include the following in an OperationStatus object:

This comment has been minimized.

@nathan-at-least

nathan-at-least Jan 18, 2017

Contributor

Change Request: Let's require that an OperationStatus always includes the method field.

@nathan-at-least

nathan-at-least Jan 18, 2017

Contributor

Change Request: Let's require that an OperationStatus always includes the method field.

@@ -98,6 +98,14 @@ def run_test (self):
else:
status = results[0]["status"]
errorString = results[0]["error"]["message"]

This comment has been minimized.

@nathan-at-least

nathan-at-least Jan 18, 2017

Contributor

Change Request: Verify that the result has a method field which equals z_sendmany.

@nathan-at-least

nathan-at-least Jan 18, 2017

Contributor

Change Request: Verify that the result has a method field which equals z_sendmany.

@@ -50,7 +50,7 @@ struct WitnessAnchorData {
class AsyncRPCOperation_sendmany : public AsyncRPCOperation {
public:
AsyncRPCOperation_sendmany(std::string fromAddress, std::vector<SendManyRecipient> tOutputs, std::vector<SendManyRecipient> zOutputs, int minDepth, CAmount fee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE);
AsyncRPCOperation_sendmany(std::string fromAddress, std::vector<SendManyRecipient> tOutputs, std::vector<SendManyRecipient> zOutputs, int minDepth, CAmount fee = ASYNC_RPC_OPERATION_DEFAULT_MINERS_FEE, Value contextInfo = Value::null);

This comment has been minimized.

@nathan-at-least

nathan-at-least Jan 18, 2017

Contributor

Comment (not change request): This API strikes me as odd, because the constructor args should be derived from contextInfo, but there's no way to enforce that. This means the caller could accidentally pass the wrong contextInfo, and this code will happily relay it.

Here are some alternatives to consider in the future:

  • pass contextInfo to the ctor then derive the other values from it. (Is this too messy because the ctor would need access to the wallet?)
  • derive the context info from the given parameters, so the RPC interface will be returning values that went through a "round trip" of parsing, verification, update. This could avoid the problem I mention above, although sometimes it's nice to know that we're seeing the original client request info, just in case that reveals malformed values that are obscured by the "round trip" processing.

For now, let's keep this design since it should work well enough.

@nathan-at-least

nathan-at-least Jan 18, 2017

Contributor

Comment (not change request): This API strikes me as odd, because the constructor args should be derived from contextInfo, but there's no way to enforce that. This means the caller could accidentally pass the wrong contextInfo, and this code will happily relay it.

Here are some alternatives to consider in the future:

  • pass contextInfo to the ctor then derive the other values from it. (Is this too messy because the ctor would need access to the wallet?)
  • derive the context info from the given parameters, so the RPC interface will be returning values that went through a "round trip" of parsing, verification, update. This could avoid the problem I mention above, although sometimes it's nice to know that we're seeing the original client request info, just in case that reveals malformed values that are obscured by the "round trip" processing.

For now, let's keep this design since it should work well enough.

@nathan-at-least

This comment has been minimized.

Show comment
Hide comment
@nathan-at-least

nathan-at-least Jan 18, 2017

Contributor

Oops, I didn't see that this was merged before beginning review. I'll file my requests as follow up tickets (so this shouldn't block release).

Contributor

nathan-at-least commented Jan 18, 2017

Oops, I didn't see that this was merged before beginning review. I'll file my requests as follow up tickets (so this shouldn't block release).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment