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

Naming improvements #3322

Merged
merged 1 commit into from Jun 12, 2018

Conversation

4 participants
@arielgabizon
Copy link
Contributor

arielgabizon commented Jun 5, 2018

  • ZCProof is a too general name, now that we also have GrothProof used in sprout proofs.
    So I changed the name of this object to PHGRProof.

  • In some files pubKeyHash was used as a var name, whereas it wasn't the pubkey hash,
    but the pubkey itself. So I changed the var name to joinSplitPubKey

@arielgabizon arielgabizon requested review from ebfull and daira Jun 5, 2018

@arielgabizon arielgabizon changed the title Rename ZCProof to PHGRProof Naming improvements & fixes Jun 6, 2018

@arielgabizon arielgabizon changed the title Naming improvements & fixes Naming improvements & small fixes Jun 6, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 7, 2018

☔️ The latest upstream changes (presumably #3237) made this pull request unmergeable. Please resolve the merge conflicts.

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Jun 7, 2018

There is no need to declare parameters of value type (e.g. uint64_t) as const; they're passed by value so there is no possibility of affecting the caller's copy. My merge undid those changes (maybe I should have done that as another commit).

@daira
Copy link
Contributor

daira left a comment

Looks good, please address my comment.

@@ -137,16 +137,18 @@ class JoinSplitCircuit : public JoinSplit<NumInputs, NumOutputs> {
} catch (...) {
return false;
}
}

}

This comment has been minimized.

@daira

daira Jun 7, 2018

Contributor

Undo this change and then squash, please.

This comment has been minimized.

@arielgabizon

arielgabizon Jun 7, 2018

Contributor

You mean also squash your merge commit?

This comment has been minimized.

@daira

daira Jun 8, 2018

Contributor

Yes.

Actually I think I made a mess by trying to resolve the conflicts with the GitHub conflict resolver tool. I'll fix it.

@daira daira force-pushed the arielgabizon:master branch from 7830040 to e5cad20 Jun 9, 2018

@daira daira changed the title Naming improvements & small fixes Naming improvements Jun 9, 2018

@daira daira dismissed their stale review Jun 9, 2018

Force-pushed to address my comments.

@daira daira requested a review from str4d Jun 9, 2018

@arielgabizon arielgabizon force-pushed the arielgabizon:master branch 2 times, most recently from aa14b28 to f6e3272 Jun 10, 2018

Improve/Fix variable names
ZCProof was too general. pubKeyHash was actually the JoinSplit pubkey
itself.

@arielgabizon arielgabizon force-pushed the arielgabizon:master branch from f6e3272 to e1a3461 Jun 10, 2018

@arielgabizon

This comment has been minimized.

Copy link
Contributor

arielgabizon commented Jun 10, 2018

@daira addressed your comment. Got into a git mess, so just repulled from master and redid the changes :)

@arielgabizon

This comment has been minimized.

Copy link
Contributor

arielgabizon commented Jun 10, 2018

Ohh oops, maybe I just redid what you did, and force pushed over a commit of yours that was good :(

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Jun 12, 2018

Yes you did. It's okay, I'll rereview now.

@daira

daira approved these changes Jun 12, 2018

Copy link
Contributor

daira left a comment

utACK (refactoring, does not need new tests)

@str4d

str4d approved these changes Jun 12, 2018

Copy link
Contributor

str4d left a comment

utACK

@str4d str4d added this to the v2.0.0 milestone Jun 12, 2018

@str4d str4d added the cleanup label Jun 12, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jun 12, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 12, 2018

📌 Commit e1a3461 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 12, 2018

⌛️ Testing commit e1a3461 with merge bdec226...

zkbot added a commit that referenced this pull request Jun 12, 2018

Auto merge of #3322 - arielgabizon:master, r=str4d
Naming improvements

- `ZCProof` is a too general name, now that we also have `GrothProof` used in sprout proofs.
So I changed the name of this object to `PHGRProof`.

- In some files `pubKeyHash` was used as a var name, whereas it wasn't the pubkey hash,
but the pubkey itself. So I changed the var name to `joinSplitPubKey`

@str4d str4d added this to In Review in Zcashd Team Jun 12, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jun 12, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing bdec226 to master...

@zkbot zkbot merged commit e1a3461 into zcash:master Jun 12, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Jun 12, 2018

@daira daira removed the request for review from ebfull Jun 12, 2018

@str4d str4d referenced this pull request Jun 19, 2018

Closed

Rename ZCProof to PHGRProof #3186

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