-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
New encoding for contract calls #5427
Conversation
8e2fb13
to
9ecc867
Compare
9bf7f45
to
3d3ed89
Compare
70cd848
to
ca811df
Compare
07cbcef
to
61f19de
Compare
Benchmark for 1ddb756Click to view benchmark
|
Benchmark for c4c70feClick to view benchmark
|
This comment was marked as spam.
This comment was marked as spam.
Benchmark for ff5bdb7Click to view benchmark
|
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 found it easier to see what's going on by looking at each commit separately, and I'm done all the way through to the one called "clean tests".
I'll continue with the rest of the commits tomorrow.
I understand this example in principle, but there seems to be things missing:
Are Is it necessary to defined |
No they are standard variables. Nothing special about them.
It is not necessary, but it is an option that we give when a contract call is made. See https://docs.fuel.network/docs/sway/sway-program-types/smart_contracts/#calling-a-smart-contract-from-a-script
|
So the examples are not self-contained, then? For instance, I see a variable |
Exactly, they are more like code snippets. |
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.
Congrats for putting all this together @xunilrj!!
I am still going through the PR and connecting the dots. The overall mechanics is already clear (contract.method()
being desugared into contract_call
that in the end calls the intrinsic) but I still didn't touch the part with the heavy code generation.
The comments posted in this review are IMO negligible compared to the overall complexity and importance of the PR and can be ignored for now if there are more pressing issues.
The only one that I think can lead to bugs is the not fully qualified name in the contract_call
function application.
I'll proceed with the review.
test/src/e2e_vm_tests/test_programs/should_pass/language/main_args/main_args_empty/src/main.sw
Outdated
Show resolved
Hide resolved
sway-core/src/semantic_analysis/ast_node/expression/typed_expression/method_application.rs
Show resolved
Hide resolved
General remark. @tritao's concern about debug symbols and loosing span information when desugaring is valid, especially in this approach when we generate the code by compiling code snippets. It looks to me we will need something like |
When I compile this simple predicate: predicate;
fn main() -> bool {
true
} the compiler panics in
|
sway-core/src/semantic_analysis/ast_node/declaration/auto_impl.rs
Outdated
Show resolved
Hide resolved
482415a
to
41a3aeb
Compare
Benchmark for 5b9eac1Click to view benchmark
|
This PR solves a problem for #5427. When compiling the Rust SDK, the type below hangs the compiler inside`find_parent`. ```sway #[allow(dead_code)] struct MegaExample<T, U> { a: ([U; 2], T), b: Vec<([EnumWGeneric<StructWTupleGeneric<StructWArrayGeneric<PassTheGenericOn<T>>>>; 1], u32)>, } ``` Now, propagating when a change is made, not only the example compiles, but we should also be able to optimize compilation in general. In small benchmarks I made here, compiling the Rust SDK had a 2% improvement (from `0m8.404s` to `0m8.258s`). ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
287d8e8
to
d201a36
Compare
Benchmark for e0acdaeClick to view benchmark
|
Benchmark for c4a74daClick to view benchmark
|
Benchmark for 900a91eClick to view benchmark
|
Benchmark for 1fd53d4Click to view benchmark
|
4814c96
to
6ddf142
Compare
Benchmark for ab66cd8Click to view benchmark
|
Benchmark for 3f74b03Click to view benchmark
|
Benchmark for 58fe54eClick to view benchmark
|
## Description This fixes a bug introduced by #5427, where the ABI was exporting the "__entry" function instead of the "main". When parsing and type checking I now mark function as "main" an only later decide if the compilation will call the main or the entry. I also moved the `is_entrypoint` method from the `TyAstNode` to the DCE pass, because it is super specific to that pass. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers.
Description
This PR implements the new encoding for contracts/scripts/predicates. #5512 and #5513
Contract Calls
When the new encoding is turned on using
--experimental-new-encoding
, contract calls like the example below will be "desugarized" differently now.and will be transformed to
And the important part is the
contract_call
function in the std library. This function does all the encoding as necessary and delegates the actual call to an intrinsic function__contract_call
. Allowing the protocol to evolve entirely in Sway.Contracts
On the other side, when the flag
--expiremental-new-encoding
is turned on, the contract specification like the one below is being transformed into all the decoding and encoding necessary.The mains points are:
__entry
that decodes the method name and its arguments. The method is selected with a bunch ofif
s at the moment, because we don´t havematch
for string slices. Then wedecode
the arguments using the correct type, which is a tuple with all the function arguments, and expand this tuple calling the function;__contract_method
;__retd
.Example:
will be transformed into
Scripts and Predicates
The protocol to call scripts and predicates will also change and will be very similar to contracts. See more above. Now when the flag is turned on, the
main
function will not be entry point anymore. The compiler will actually generate an__entry
function that will decode arguments and encode the result, like contracts.For example:
will be transformed into
Tests
To facilitate testing this PR introduces three changes to our test harness:
1 - A new parameter can be called to update test output files (abi and storage json). This facilitates when we only want to copy and paste these output files to their respective oracles. Brings the framework closer to a snapshot one.
2 - Depending on the configuration at
test.toml
multiple executions of the same test will be scheduled. At the moment, tests that depend on the encoding will run with the--experimental-new-encoding
flag automatically. For example:3 - A new
script_data_new_encoding
was created because we want to support and run tests with the two encoding for a time. This is also what flags tests to run with the flag on automatically.Checklist
Breaking*
orNew Feature
labels where relevant.