-
Notifications
You must be signed in to change notification settings - Fork 828
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
CT types - Apply balance close account #1919
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/ct_types #1919 +/- ##
====================================================
- Coverage 61.37% 61.25% -0.12%
====================================================
Files 267 267
Lines 23798 23906 +108
====================================================
+ Hits 14606 14644 +38
- Misses 8133 8190 +57
- Partials 1059 1072 +13
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with minor suggestions:
RE the validate suggestion, I can work on that when I merge the PRs if you think that makes sense too.
message MsgCloseAccount { | ||
string address = 1; | ||
string denom = 2; | ||
CloseAccountProof proof = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this proofs instead for consistency
} | ||
|
||
func (c *CloseAccountProof) Validate() error { | ||
if c.ZeroAvailableBalanceProof == nil || c.ZeroPendingBalanceLoProof == nil || c.ZeroPendingBalanceHiProof == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought: Should we make the validate() calls chain validate the child proofs here as well (instead of just checking for nil). Seems like it would be what the validate call should do instead of making that the responsibility of FromProto.
return nil | ||
} | ||
|
||
func (z *ZeroBalanceProof) ToProto(zkp *zkproofs.ZeroBalanceProof) *ZeroBalanceProof { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not actional for this PR but i'm planning to rework all the ToProto calls to make them just regular constructors NewZeroBalanceProof(zkp*zkproofs.ZeroBalanceProof) *ZeroBalanceProof
instead of a instance method of z, since it doesn't actually use z.
* pending balance type * convert apply pending balance to a message type * close account type
Describe your changes and provide context
Testing performed to validate your change