Use firefly-signer library and allow numbers expressed as strings #837

nguyer · 2022-05-27T17:48:56Z

This PR introduces firefly-signer's ABI parsing functions and uses them in FireFly's ABI -> FFI generation code. It also replaces any existing ABI data structures that were originally copied over from Ethconnect, with those of firefly-signer.

This PR also makes some enhancements to the ABI -> FFI generation code itself to allow numbers to be expressed in a JSON string or a JSON number. This is reflected in the generated OpenAPI Spec for custom contracts, including a description in the Swagger UI that describes this.

Further improvements have also been made to the OpenAPI Spec / Swagger UI for custom smart contracts, to indicate if a function is payable, etc. The options object also appears in the schema to make it more obvious how to pass in additional options.

A new details field has been added to FFIMethod and FFIEventDefinition structs. In the same way that a details field has always wrapped blockchain specific information for a FFIParam, we now have this ability on a function and event level as well. For an Ethereum smart contract, this is used to indicate whether a function is payable, whether an event is anonymous, etc. The details are simply stored as a fftypes.JSONObject. In the rendered Swagger UI, a table is constructed to show each field and its corresponding value in the Function / Event description.

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

peterbroadhurst

@nguyer - I paused the review when I hit quite a big question, about whether you'd used the functions introduced in the ABI parsing structure specifically to handle features like OpenAPI generation. Could you take a look and see if there's more to do, either in Core, or in firefly-signer, to ensure identical recursive processing on the metadata generation and runtime?

internal/blockchain/ethereum/ethereum.go

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

codecov-commenter · 2022-06-10T20:59:00Z

Codecov Report

Merging #837 (e7b99a9) into main (6ad84ee) will not change coverage.
The diff coverage is 100.00%.

❗ Current head e7b99a9 differs from pull request most recent head 166272b. Consider uploading reports for the commit 166272b to get more accurate results

@@            Coverage Diff            @@
##              main      #837   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          301       301           
  Lines        19718     19797   +79     
=========================================
+ Hits         19718     19797   +79

Impacted Files	Coverage Δ
pkg/core/ffi.go	`100.00% <ø> (ø)`
internal/blockchain/ethereum/ethereum.go	`100.00% <100.00%> (ø)`
internal/blockchain/ethereum/eventstream.go	`100.00% <100.00%> (ø)`
...nternal/blockchain/ethereum/ffi_param_validator.go	`100.00% <100.00%> (ø)`
pkg/core/ffi_param_validator.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ad84ee...166272b. Read the comment docs.

nguyer · 2022-06-10T21:25:18Z

Just realized there are some major merge conflicts here, including some changes that actually need to go into firefly-common. I'll sort those before requesting a re-review.

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

peterbroadhurst · 2022-06-14T13:42:32Z

internal/apiserver/ffi2swagger.go

 func (og *ffiSwaggerGen) addMethod(routes []*ffapi.Route, method *core.FFIMethod, hasLocation bool) []*ffapi.Route {
+	description := method.Description
+	if method.Details != nil && len(method.Details) > 0 {
+		description = fmt.Sprintf("%s\n\nAdditional smart contract details:\n\n%s", description, buildDetailsTable(method.Details))


Struggling a little to understand this. What is the end to end of the details? Is it the place where devdocs would be specified?

One specific item (along with the wider question) is the Additional smart contract details string needs to be translated.

Sure! I did forget to mention that in the description, sorry about that. The short version is that it's a way to bubble up blockchain specifics about a function, like if it's payable or not.

Good call out on the string translation, thanks.

…t docs. Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

peterbroadhurst · 2022-06-14T15:33:13Z

internal/blockchain/ethereum/ethereum.go

+			}
+		}
+		if payable, ok := method.Details["payable"]; ok {
+			abiEntry.Payable = payable.(bool)


I note this will mean we panic if someone puts {"payable":"true"} into the field. I wonder if it makes sense to use the JSONObject field getters here?

Ah good suggestion. I'll change this one and the similar checks for other fields.

peterbroadhurst · 2022-06-14T15:38:28Z

internal/blockchain/ethereum/ethereum.go

+	}
+	switch typeComponent.ComponentType() {
+	case abi.ElementaryComponent:
+		t := e.getFFIType(typeComponent.ElementaryType().String())


I note it was intended you could compare these against the list of elementary types here:
https://pkg.go.dev/github.com/hyperledger/firefly-signer/pkg/abi#pkg-variables

(rather than needing to keep a separate list of strings to compare to)

I think we still need this bit of code, because somewhere we need a mapping from ABI type (e.g. uint256) to the JSON input type we will accept on the API (e.g. integer)

peterbroadhurst · 2022-06-14T15:43:16Z

internal/blockchain/ethereum/ffi_param_validator.go

 var intRegex, _ = regexp.Compile("^u?int([0-9]{1,3})$")
 var bytesRegex, _ = regexp.Compile("^bytes([0-9]{1,2})?")

 func (v *FFIParamValidator) Compile(ctx jsonschema.CompilerContext, m map[string]interface{}) (jsonschema.ExtSchema, error) {


I was trying to work out where this is used. I think we agreed that the connector would be responsible for verifying input, and that we should ensure that happens synchronously before we give back a 202 to an invoke.

Understand this PR might not be the final step in the chain, but I want to make sure we're not trying to replicate the connector input validation logic in core.

The most important use of this is in the contracts manager

firefly/internal/contracts/manager.go

Lines 112 to 118 in e841984

func (cm *contractManager) newFFISchemaCompiler() *jsonschema.Compiler {

c := core.NewFFISchemaCompiler()

if cm.ffiParamValidator != nil {

c.RegisterExtension(cm.ffiParamValidator.GetExtensionName(), cm.ffiParamValidator.GetMetaSchema(), cm.ffiParamValidator)

}

return c

}

Once this JSON Schema extension is registered, the JSON Schema library calls this function when a request is made to broadcast a new FFI. This ensures that the schema specified in the FFI for each field adheres to the specific rules defined by each blockchain connector. As an example, the Ethereum connector requires the details object in most places, and it requires that a type field is set inside there (in addition to a bunch of other schema rules).

In other words, this is the metaschema (the schema for the schema). This code is not for validating blockchain invocation request input.

peterbroadhurst

Couple of comments for consideration before merge @nguyer - but this looks great!

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

nguyer requested review from awrichar and peterbroadhurst as code owners May 27, 2022 17:48

nguyer force-pushed the abi-refactoring branch from 7bbcc59 to 152914e Compare May 27, 2022 17:50

nguyer requested a review from nickgaski as a code owner May 27, 2022 17:50

nguyer added 2 commits May 31, 2022 10:50

Use firefly-signer for ABI handling

b24657d

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

Support integer input as JSON string or integer

e7b99a9

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

nguyer force-pushed the abi-refactoring branch from 152914e to e7b99a9 Compare May 31, 2022 14:50

peterbroadhurst requested changes May 31, 2022

View reviewed changes

internal/blockchain/ethereum/ethereum.go Outdated Show resolved Hide resolved

Add details to FFIFunctions and FFIEvents

166272b

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

nguyer added 3 commits June 13, 2022 10:17

Merge branch 'main' into abi-refactoring

879844c

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

Update generated oas for custom contracts

85ae9c8

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

Merge branch 'main' into abi-refactoring

6c8dce6

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

peterbroadhurst reviewed Jun 14, 2022

View reviewed changes

Add string translations for FFI method details. Update custom contrac…

e841984

…t docs. Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

peterbroadhurst reviewed Jun 14, 2022

View reviewed changes

peterbroadhurst approved these changes Jun 14, 2022

View reviewed changes

Improve FFI details parsing

9aca617

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>

nguyer merged commit 15c8802 into hyperledger:main Jun 14, 2022

nguyer deleted the abi-refactoring branch June 14, 2022 16:58

nguyer mentioned this pull request Jun 20, 2022

FFI parameters with arrays of objects don't get converted correctly to Ethereum ABI #704

Closed

	func (cm contractManager) newFFISchemaCompiler() jsonschema.Compiler {
	c := core.NewFFISchemaCompiler()
	if cm.ffiParamValidator != nil {
	c.RegisterExtension(cm.ffiParamValidator.GetExtensionName(), cm.ffiParamValidator.GetMetaSchema(), cm.ffiParamValidator)
	}
	return c
	}

Use firefly-signer library and allow numbers expressed as strings #837

Use firefly-signer library and allow numbers expressed as strings #837

Uh oh!

Conversation

nguyer commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peterbroadhurst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Jun 10, 2022

Codecov Report

Uh oh!

nguyer commented Jun 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbroadhurst left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nguyer commented May 27, 2022 •

edited

Loading