Skip to content
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

Make 08-wasm contract api types exported #4913

Closed
3 tasks
damiannolan opened this issue Oct 19, 2023 · 4 comments
Closed
3 tasks

Make 08-wasm contract api types exported #4913

damiannolan opened this issue Oct 19, 2023 · 4 comments
Assignees
Labels

Comments

@damiannolan
Copy link
Member

damiannolan commented Oct 19, 2023

Summary

Consider making the 08-wasm contract API types exported/public.

Problem Definition

The 08-wasm module defines a number of structs used for encoding the contract payloads to JSON.
It is not necessarily the types themselves but the serialized JSON bytes of these types which define the API, as contracts are not written natively in Go and rather in Rust. Contracts written in Rust will likely define their own types to which they unmarshal to or consume some common library.

We currently need to export these types in export_test.go in order to expose them for testing. Note, that this is the reason the mock wasm engine implementation lives still within the types package. When implementing some test cases I stumbled upon this and would have expected for the mock wasm engine to live within the 08-wasm/testing package.

At the moment I am struggling to see the value in keeping these types unexported / private. If anyone can highlight some good points or concerns about making them exported we should discuss them here. I believe it was also pointed out that this API is final and can only be extended in future, which suggests further than there is little value in maintaining them as private structs.

Proposal

Make the types defined in contract_api.go public / exported symbols. With this we can then move the mock wasm engine implementation to the testing package and remove export_test.go and the overhead associated with this for testing.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@damiannolan damiannolan added needs discussion Issues that need discussion before they can be worked on 08-wasm labels Oct 19, 2023
@damiannolan damiannolan added this to the 08-wasm/v0.1.0 milestone Oct 19, 2023
@DimitrisJim
Copy link
Contributor

DimitrisJim commented Oct 19, 2023

I'd say the main reason I see is not requiring a major/minor bump any time we might make changes to the Go types. We might make some changes to go structs that don't affect contract API but do change 08-wasm's public API if that is exposed (i.e #4909).

I'd be more favorable if we make all their fields private, if possible (and definitely release 08-wasm with a < 1.0.0 version)

@damiannolan
Copy link
Member Author

Unfortunately I don't think we can make the fields private afaik, as encoding/json requires them to be public

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Oct 20, 2023

Yea, that seems to be the case. It seems you can create a dummy private struct that holds the exported fields and embed that into the struct you want to marshal/unmarshal but that seems like an equally annoying workaround.

@damiannolan damiannolan removed the needs discussion Issues that need discussion before they can be worked on label Oct 23, 2023
@damiannolan damiannolan self-assigned this Oct 23, 2023
@DimitrisJim
Copy link
Contributor

closed in #4927 another issue will be opened for moving mock engine into testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

2 participants