-
Notifications
You must be signed in to change notification settings - Fork 242
Add endpoint to automatically generate an FFI #511
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
Conversation
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
| {Name: "ns", ExampleFromConf: config.NamespacesDefault, Description: i18n.MsgTBD}, | ||
| }, | ||
| QueryParams: []*oapispec.QueryParam{ | ||
| {Name: "confirm", Description: i18n.MsgConfirmQueryParam, IsBool: true, Example: "true"}, |
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.
Seems like this is unused?
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.
Yep. Good catch, thanks.
internal/contracts/manager.go
Outdated
| GetContractSubscriptionByNameOrID(ctx context.Context, ns, nameOrID string) (*fftypes.ContractSubscription, error) | ||
| GetContractSubscriptions(ctx context.Context, ns string, filter database.AndFilter) ([]*fftypes.ContractSubscription, *database.FilterResult, error) | ||
| DeleteContractSubscriptionByNameOrID(ctx context.Context, ns, nameOrID string) error | ||
| GenerateFFI(ctx context.Context, ns, name, version string, input *fftypes.JSONAny) (*fftypes.FFI, 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.
Any reason we pass name and version here? Seems like the only usage of this method always sets them to empty strings.
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.
Yeah... maybe worth a quick chat about this one... I wrote the backend first and thought that it would make sense to pass in the name, version, etc. so it could be filled in on the generated FFI. Then I created the API endpoint and couldn't think of a good place to specify them when calling the API, because the payload of that request right now is entirely an ABI array.
One option would be to modify the endpoint to wrap the ABI elements in another object so that request would look like:
{
"name": "...",
"version": "...",
"abi": [...]
}| return ctx.GetStub().PutState(name, assetJSON) | ||
| } | ||
|
|
||
| func (s *SmartContract) GetAsset(ctx contractapi.TransactionContextInterface, name string) (string, 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.
No objections to this being added to the chaincode... but was it intended to be used in a test or something? Seems unused at this point.
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.
Ah this is being used in my other branch for additional tests :) It shouldn't have been in this commit. I'll remove it now for clarity.
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Codecov Report
@@ Coverage Diff @@
## main #511 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 275 276 +1
Lines 15731 15922 +191
==========================================
+ Hits 15731 15922 +191
Continue to review full report at Codecov.
|
internal/contracts/manager.go
Outdated
|
|
||
| func (cm *contractManager) GenerateFFI(ctx context.Context, ns string, generationRequest *fftypes.FFIGenerationRequest) (*fftypes.FFI, error) { | ||
| if generationRequest.Namespace == "" { | ||
| generationRequest.Namespace = ns |
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.
I think in general we ignore any nested Namespace value in an input body (ie list it in "ignored" fields so that Swagger won't generate it), and always write/overwrite with the one from the path.
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.
Ah I didn't realize that. I did the exact opposite here. I'll switch it around to match the existing pattern. Thanks!
awrichar
left a comment
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.
Looks good; I think the new input body format is a lot better (and more future-proof). One minor comment left above.
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
This PR adds an endpoint to automatically generate an FFI. The implementation is handled by the blockchain plugin. Right now, only the Ethereum plugin implements this functionality, but other blockchain plugins in the future could as well, if they have their own DSL to describe a smart contract.
Resolves #397