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

__log outputting encoded values #5306

Merged
merged 46 commits into from
Jan 15, 2024
Merged

__log outputting encoded values #5306

merged 46 commits into from
Jan 15, 2024

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Nov 28, 2023

Description

This PR changes the output of __log to encoded values (see #4769). A quick example of what that means is

struct S {
    a: u64,
    b: u32,
    c: u16,
    d: u8,
    e: Vec<u64>,
    f: str,
    g: u256
}

let mut e = Vec::new();
e.push(1);
e.push(2);
e.push(3);

__log(S{
    a: 1,
    b: 2,
    c: 3,
    d: 4,
    e,
    f: "sway",
    g: u256::max()
});

will output

 [0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 2, 0, 3, 4, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 4,    115,   119,    97,   121, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255]
  ^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^  ^^^^  ^  ^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^^^     ^^^    ^^^     ^^    ^^^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     s.a         s.b   s.c s.d              s.e.len()                  s.e[0]                  s.e[1]                  s.e[2]               s.f.len()  s.f[0] s.f[1] s.f[2] s.f[3]  s.g

This works in two steps:

1 - __log(s) is desugared into __log(encode(s));
2 - call encode. Its impl can be found at https://github.com/FuelLabs/sway/pull/5306/files#diff-ee5cebc963e841e8af05f3986de17dd266ee6e9b49dbe089a5eb64764f3b802eR307

It simply creates an append-only buffer and call abi_encode from a special trait named AbiEncode.

To be encodable, a type must implement AbiEncode. In the example above, S is auto-implemented by the compiler because all its fields are AbiEncode. But we can also have custom impl like Vec here: https://github.com/FuelLabs/sway/pull/5306/files#diff-b5d9688741fea479477f26ca44cd1d1ecbd2f003f3875292abb23df7fad85c58

All this is behind a compiler flag:

> forc build -h
Compile the current or target project

USAGE:
    forc build [OPTIONS]

OPTIONS:
...
        --experimental-new-encoding
            Experimental flags for the "new encoding" feature

The same flag is available for the e2e tests:

> cargo r -p test --release -- should_pass/language/logging --verbose --experimental-new-encoding

Limitations

1 - Now that __log demands a fn called encode, when something is compiled with implicit-std: false, the intrinsic function __log does not work out-of-box anymore. A function called encode must "visible" and the worst part is that it needs to be functional to log anything.

2 - Arrays, string arrays and tuples will have limited implementations. Currently up to five items.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@xunilrj xunilrj mentioned this pull request Nov 29, 2023
10 tasks
@xunilrj xunilrj force-pushed the xunilrj/trait-auto-impl branch 2 times, most recently from a759b88 to 34e3ad8 Compare December 5, 2023 20:01
@xunilrj xunilrj self-assigned this Dec 12, 2023
@xunilrj xunilrj changed the title AbiEncode trait auto implementation __log outputting encoded values Dec 13, 2023
@iqdecay iqdecay self-requested a review December 18, 2023 16:21
@xunilrj
Copy link
Contributor Author

xunilrj commented Dec 20, 2023

Some tests are breaking because of #5383. The problem is that when we generate the auto impl, it ends up resolving to the wrong types.

@xunilrj
Copy link
Contributor Author

xunilrj commented Dec 20, 2023

The task to test std-lib does not work, because it depends on the SDK parsing the log correctly. Something we still need to do.

xunilrj pushed a commit that referenced this pull request Dec 22, 2023
## Description

Closes #5383.

#5383 blocks PR #5306.

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] 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.
- [ ] 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.
@xunilrj xunilrj marked this pull request as ready for review December 23, 2023 20:00
@jjcnn
Copy link
Contributor

jjcnn commented Jan 5, 2024

I'm not sufficiently familiar with most of the code being touched here, so I don't feel comfortable giving a review a such.

I did want to raise the issue of whether it's possible to go from the log output back to the data? The type of the data doesn't seem to be part of the encoding, so to map the log back to the data it came from you would need to supply the types separately.

@xunilrj
Copy link
Contributor Author

xunilrj commented Jan 5, 2024

whether it's possible to go from the log output back to the data?

Only looking at the bytes... no. But this was a explicit decision (that we can discuss if is a good one or not).
For that, and specifically for logging, you will have to look ate the ABI JSON.

"loggedTypes": [
    {
      "logId": 0,
      "loggedType": {
        "name": "",
        "type": 5,
        "typeArguments": []
      }
    },
    ...

The test that test this is the one broken because we need to update the Rust SDK. What we do in that test is running a macro on top on this JSON that "recreate" all the types and give you easy access to all typed logged data.

Unfortunately both repos are dependent on one another. :(

@xunilrj xunilrj requested a review from a team January 11, 2024 00:39
Copy link
Contributor

@IGI-111 IGI-111 left a comment

Choose a reason for hiding this comment

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

A few nitpicks but LGTM

Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

Looks good, excited to see this land.

@JoshuaBatty JoshuaBatty requested review from a team January 15, 2024 01:23
@JoshuaBatty JoshuaBatty added the enhancement New feature or request label Jan 15, 2024
@tritao tritao merged commit cab2db3 into master Jan 15, 2024
35 checks passed
@tritao tritao deleted the xunilrj/trait-auto-impl branch January 15, 2024 11:26
xunilrj added a commit that referenced this pull request Jan 26, 2024
…5481)

## Description

This PR is a continuation of #5306.

- I am fixing some code reviews issues that were raised in the other PR;
- I am incorporating the encoding version inside the JSON ABI as:

```json
          {
            "configurables": [],
             "encoding": "1",  <- look here
            "functions": [
              {
                "attributes": null,
                "inputs": [],
                "name": "main",
                "output": {
                  "name": "",
                  "type": 13,
                  "typeArguments": null
                }
              }
            ],
```

This field is a string to allow any kind of versioning we choose.

- This PR has also improved testing and making more explicit how each
type is being encoded.
## Dependencies

- [x] FuelLabs/fuel-abi-types#17

## 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.
xunilrj added a commit that referenced this pull request Feb 1, 2024
…5481)

## Description

This PR is a continuation of #5306.

- I am fixing some code reviews issues that were raised in the other PR;
- I am incorporating the encoding version inside the JSON ABI as:

```json
          {
            "configurables": [],
             "encoding": "1",  <- look here
            "functions": [
              {
                "attributes": null,
                "inputs": [],
                "name": "main",
                "output": {
                  "name": "",
                  "type": 13,
                  "typeArguments": null
                }
              }
            ],
```

This field is a string to allow any kind of versioning we choose.

- This PR has also improved testing and making more explicit how each
type is being encoded.
## Dependencies

- [x] FuelLabs/fuel-abi-types#17

## 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.
@xunilrj xunilrj mentioned this pull request Feb 1, 2024
14 tasks
xunilrj added a commit that referenced this pull request Feb 1, 2024
…5481)

This PR is a continuation of #5306.

- I am fixing some code reviews issues that were raised in the other PR;
- I am incorporating the encoding version inside the JSON ABI as:

```json
          {
            "configurables": [],
             "encoding": "1",  <- look here
            "functions": [
              {
                "attributes": null,
                "inputs": [],
                "name": "main",
                "output": {
                  "name": "",
                  "type": 13,
                  "typeArguments": null
                }
              }
            ],
```

This field is a string to allow any kind of versioning we choose.

- This PR has also improved testing and making more explicit how each
type is being encoded.

- [x] FuelLabs/fuel-abi-types#17

- [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.
IGI-111 added a commit that referenced this pull request Mar 10, 2024
## Description
Enables a did_change stress test with random wait times between each
trigger to emulate random key typing. This then internally kicks off
compilation and triggers garbage collection every 3 keystrokes.

I had intended to include this test when garbage collection was
implemented in #5251. However, at that time, we were only performing GC
every 10th keystroke and it was overloading the stack in CI. Now that we
have reduced this to every 3rd keystroke it seems to be fine for CI.

Unfortunately, this test wasn't running to catch a bug that slipped
through in #5306. As such, this PR currently allows a way for us to
reproduce this error but won't be able to pass CI until the underlying
issue is resolved.

P.S.: This also adds a temporary fix where we retain elements without a
source id. This is tantamount to leaking them but seems preferable to
disabling the GC altogether.

---------

Co-authored-by: xunilrj <xunilrj@hotmail.com>
Co-authored-by: IGI-111 <igi-111@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants