-
Notifications
You must be signed in to change notification settings - Fork 9
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
API refactoring + missing features #16
Conversation
8f8396b
to
05d568f
Compare
Hi @damip we need a review and validation after the cleanup before starting the implementations in Massa core. We could see later the private service which is lot simpler than the public one, If we could validate It together, It'll speed up our developments |
string message = 2; | ||
} | ||
|
||
// Empty |
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.
IIRC this type won't be used anymore on the ABI side when we will merge.
do you use it in any API?
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.
don't mind, I've found where you use it.
I though you use dedicated empty types?
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 used in the API, so we could keep It at the moment. We created our Empty and Error type to not rely on google ones
@@ -13,6 +13,17 @@ option php_namespace = "Com\\Massa\\Model\\V1"; | |||
option ruby_package = "Com::Massa::Model::V1"; | |||
option swift_prefix = "MMODEL"; | |||
|
|||
// Massa error | |||
message Error { |
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.
We also have an Error message on ABI side.
Where not currently using any code field, do you ?
I'm asking to prevent clash when we will merge
How do you discriminate 0 from None?
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.
We could share the same structure, the errors codes will be written later
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.
Yes we probably should share this type (and possibly others).
We need to sync a bit more maybe merging common types in a dedicated pr then having our own pr relying on it
I'm gonna open a pr on what we have on the abi side for you to see
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.
The only issue is : NativeAmount
which is not present in testnet 24
It's time to merge this version to main in order to prepare the newer gRPC API |
This reverts commit 962de24.
bf2b57a
to
b055997
Compare
This PR should contains a pre prod version of our gRPC API, starting from this version, we'll try to avoid breaking changes to make the life of builders easier and they'll be able to use the latest version of Massa in the buildnet
closes massalabs/massa#4187