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

[Go] [Rust] update language bindings to return value API #417

Merged
merged 26 commits into from
Jul 5, 2024
Merged

Conversation

Vindaar
Copy link
Collaborator

@Vindaar Vindaar commented Jul 2, 2024

Previously the API for Go and Rust simply followed the C API (and by extension the Nim API from which the C API is generated) almost literally. That meant return types were reserved for error codes (or enums) and all 'interesting' data was passed via mutable arguments.

This changes the API to return values instead. This has the nice advantage on the Rust side that it sidesteps the MaybeUninit issue mentioned in #387.

Note: currently the CI does not pass, because of the bug fixed in #409. There might be other minor issues present, but due to "expected" test failures, I haven't dug into every current failure for Go/Rust.

mratsim
mratsim previously approved these changes Jul 2, 2024
Copy link
Owner

@mratsim mratsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, lgtm, some nits

constantine-go/constantine.go Show resolved Hide resolved
constantine-go/constantine.go Show resolved Hide resolved
constantine-go/constantine.go Show resolved Hide resolved
constantine-go/constantine.go Show resolved Hide resolved
constantine-go/constantine.go Show resolved Hide resolved
constantine-go/constantine.go Outdated Show resolved Hide resolved
}
}

fn t_generate<T: CompareReturn, F>(test_name: String, func: F)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn t_generate<T: CompareReturn, F>(test_name: String, func: F)
fn t_generate<T: impl Iterator<Item = u8>, F>(test_name: String, func: F)

here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While your proposal is not quite correct (impl Iterator is not valid syntax must be without the impl), it led me down the right path. Iterator would require the test functions to actually return an iterator. But there's also a predefined IntoIterator trait, which works (via x.into_iter().collect::<Vec<u8>>()).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, the impl Iterator is for arguments, not generics. And yes Vec use into_iter indeed.

@@ -60,6 +60,31 @@ func eth_evm_sha256*(r: var openArray[byte], inputs: openArray[byte]): CttEVMSta
sha256.hash(cast[ptr array[32, byte]](r[0].addr)[], inputs)
return cttEVM_Success

func modexp_result_size*(size: var uint64, inputs: openArray[byte]): CttEVMStatus {.noInline, tags:[Alloca, Vartime], libPrefix: prefix_ffi, meter.} =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func modexp_result_size*(size: var uint64, inputs: openArray[byte]): CttEVMStatus {.noInline, tags:[Alloca, Vartime], libPrefix: prefix_ffi, meter.} =
func eth_evm_modexp_result_size*(size: var uint64, inputs: openArray[byte]): CttEVMStatus {.noInline, tags:[Alloca, Vartime], libPrefix: prefix_ffi, meter.} =

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure whether to reuse the naming convention. I didn't so that there is a clearer distinction between what is a real EVM precompiles and what is a helper function. Will change it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's namespacing in case several protocols have differing modexp, say a modexp for the EVM, a modexp to simulate GMP, etc ...

* The associated modulus length in bytes is the size required by the
* result to `eth_evm_modexp`.
*/
ctt_evm_status ctt_modexp_result_size(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ctt_evm_status ctt_modexp_result_size(
ctt_evm_status ctt_eth_evm_modexp_result_size(

mratsim and others added 20 commits July 5, 2024 14:41
NOTE: currently not all tests pass, due to the unmarshaling bug. We
will rebase once a final decision for the solution has been made.
Adds a couple of overlooked API functions,
- validation (sec, pub, sig)
- serialization of seckeys
- deserialize unchecked
@Vindaar
Copy link
Collaborator Author

Vindaar commented Jul 5, 2024

Both Go and Rust should now pass the tests. We'll see.

Copy link
Owner

@mratsim mratsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks a lot

@mratsim mratsim merged commit 0fe6bbc into master Jul 5, 2024
24 checks passed
@mratsim mratsim deleted the lang-api branch July 5, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants