search

makerdao / dss-test

GNU Affero General Public License v3.0
11 stars 9 forks source link

Use vm json memory object to persist logs as it is much faster than storage #31

Closed gbalabasquer closed 1 year ago

gbalabasquer commented 1 year ago

In my case, running the IntegrationTest cases in master takes ≈ 90 seconds. The same tests in this branch takes ≈ 30 seconds.

hexonaut commented 1 year ago

Actually, I'm struggling to understand why the first 64 bytes are being removed from the data. Could you elaborate on that?

gbalabasquer commented 1 year ago

Your changes look good Gonza. I just realized that the logs expose the event emitter! This is great it allows us to differentiate the Optimism L1 and L2 events for both Bedrock and legacy versions. I hope you don't mind, but I've committed that fix as well and enabled storing the event emitter. Feel free to merge if my commit looks good to you.

That's good news, great!

Actually, I'm struggling to understand why the first 64 bytes are being removed from the data. Could you elaborate on that?

So I was having errors with abi.decode due to a 64 bytes prefix being added when importing bytes content. One of those extra bytes was the length of the bytes, the other I think it was the position where the bytes content starts. It was like it was interpreting the bytes meta data as its content. It was easier to notice using actual files as I could see in the json the real content but when importing them, those two extra words been added. There is a weird thing now though, if I just keep the whole bytes with the extra 64, all the tests are passing, excepting the two new, and those two seems to be failing due to different things (one is a proper revert the other a memory allocation overflow). I think something has changed when stopped using the export/import of a proper file. I'll dig a bit deeper to understand why now just in these two cases fail when keeping the first 2 words.

gbalabasquer commented 1 year ago

@hexonaut I understand the difference now. When I did the last changes, I made that the newest logs are being saved in the json structure and also assigned to the returned variable directly (check last for of getLogs). Before it was adding everything in addLogs and then reading from the saved structure everything (regardless was new or old log). In the case of new logs, the bytes doesn't get "malformed" as we are using the one coming from vm.getRecordedLogs() directly. That's why, now only the two new test cases, are failing without removing the first two words (as are the only ones using the "old" logs).

And about the issue itself, this is an example of what is coming if logging log.data (right before using abi.decode in relay functions):

0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000c00000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000001d1499e622d69689cdf9004d05ec547d650ff21100000000000000000000000000000000000000000000000000000000000000600000000000000000000000000000000000000000000000000000000000000024959ac484000000000000000000000000000000000000000000000000000000000000000300000000000000000000000000000000000000000000000000000000

We can see the metadata of position and size is interpreted as part of the content. This might be probably some type of bug in vm.parseJson or maybe it is just an expected behavior, not sure.

hexonaut commented 1 year ago

Ah I see the issue. You need to abi.decode() to bytes same as the other parsing. You can use StdJson library btw for all this parsing: https://github.com/foundry-rs/forge-std/blob/master/src/StdJson.sol#L85

Can you make that fix? Then this is good to merge imo.

gbalabasquer commented 1 year ago

Ah I see the issue. You need to abi.decode() to bytes same as the other parsing. You can use StdJson library btw for all this parsing: https://github.com/foundry-rs/forge-std/blob/master/src/StdJson.sol#L85

Can you make that fix? Then this is good to merge imo.

This didn't work for me, that's why I actually haven't used StdJson, as having this issue with the bytes. Tried again and getting a bunch of test errors.

gbalabasquer commented 1 year ago

Ah I see the issue. You need to abi.decode() to bytes same as the other parsing. You can use StdJson library btw for all this parsing: https://github.com/foundry-rs/forge-std/blob/master/src/StdJson.sol#L85 Can you make that fix? Then this is good to merge imo.

This didn't work for me, that's why I actually haven't used StdJson, as having this issue with the bytes. Tried again and getting a bunch of test errors.

Ok yeah it works if I keep the if. I'm gonna do some changes and push the commit.