-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add proto definitions for Native DMC RPC client #19
Conversation
protobuf/types/dmc.proto
Outdated
} | ||
|
||
message GetBlockHashParam { | ||
option uint64 at = 1; |
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.
It's optional
and it's not supported by proto3.
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.
How would I deal with rust option values in proto?
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.
Proto only supports a default value and default value for uint64
is zero, which is what will be assumed to be empty. If zero has special meaning in this context and integer size is not a problem, then we could use int64
and -1
to represent empty value.
protobuf/types/dmc.proto
Outdated
string signature = 4; | ||
} | ||
|
||
message MintBlockParam { |
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.
Let's use Input
instead of Param
and Result
instead of Return
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.
Ok
protobuf/types/dmc.proto
Outdated
message DNCTx { | ||
string from = 1; | ||
string to = 2; | ||
uint64 amount = 3; | ||
string signature = 4; | ||
} | ||
|
||
message DMCTx { | ||
string from = 1; | ||
string to = 2; | ||
uint64 amount = 3; | ||
string signature = 4; | ||
} |
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.
Will these two messages have the same set of fields? If so, we can combine them?
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.
Yea, they probably would. But need an identifier to distinguish between DMC side tx and Native side tx.
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.
Why do we need to distinguish them if they have the same structure? Can't we just have Transaction
?
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.
Because the address format from both sides will be different.
- Make DMCTx and DNCtx into one message with an identifier.
What kind of PR is this?:
/kind feature
What this PR does / why we need it:
Add proto definitions for the parameters involved with the RPC calls to the substrate DMC.
Which issue(s) does this PR fixes?:
Fixes #
Additional comments?: