fix: check state changes

[gzeoneth]

Was #124 reverted by #125

Handling a case where when the diff is a decoded mapping, the original object can be null if the key was not present in the original state. Example:

Uniswap prop 11

{
  address: '0x4976fb03c32e5b8cfe2b6ccb31c09ba78ebaba41',
  soltype: {
    name: 'texts',
    type: 'mapping (bytes32 => mapping (string => string))',
    storage_location: 'storage',
    offset: 0,
    index: '0x0000000000000000000000000000000000000000000000000000000000000009',
    indexed: false
  },
  original: null,
  dirty: {
    '0xa35d592ec6e5289a387cba1d5f82be794f495bd5a361a1fb314687c6aefea1f4': {
      'Voltz Uni v3 Additional Use Grant': 'CiAgICBWb2x0eiBMYWJzIFRlY2hub2xvZ3kgTGltaXRlZCAo4oCcVm9sdHrigJ0pIGlzIGdyYW50ZWQgYW4gYWRkaXRpb25hbCB1c2UgZ3JhbnQgdG8gYWxsb3cgdGhlIFZvbHR6IERBTyB0byB1c2UgdGhlIFVuaXN3YXAgVjMgQ29yZSBzb2Z0d2FyZSBjb2RlICh3aGljaCBpcyBtYWRlIGF2YWlsYWJsZSB0byBWb2x0eiBzdWJqZWN0IHRvIGxpY2Vuc2UgYXZhaWxhYmxlIGF0IGh0dHBzOi8vZ2l0aHViLmNvbS9Vbmlzd2FwL3YzLWNvcmUvYmxvYi9tYWluL0xJQ0VOU0UgKHRoZSDigJxVbmlzd2FwIENvZGXigJ0pKS4gIAkKICAgIEFzIHBhcnQgb2YgdGhpcyBhZGRpdGlvbmFsIHVzZSBncmFudCwgdGhlIFZvbHR6IERBTyByZWNlaXZlcyBhIGxpbWl0ZWQgd29ybGR3aWRlIGxpY2Vuc2UgdG8gdXNlIHRoZSBVbmlzd2FwIENvZGUgZm9yIHRoZSBwdXJwb3NlcyBvZjoKICAgIGNyZWF0aW5nLCBkZXBsb3lpbmcgYW5kIG1ha2luZyBhdmFpbGFibGUgYXNwZWN0cyBvZiBhbiBpbnRlcmVzdCByYXRlIHN3YXAgYXV0b21hdGVkIG1hcmtldCBtYWtlciAodGhlIOKAnElSUyBBTU3igJ0pOyAKICAgIHRvIG1vZGlmeSBhbmQgdXBkYXRlIHRoZSBJUlMgQU1NIG92ZXIgdGltZTsgYW5kIAogICAgZGVwbG95IHRoZSBJUlMgQU1NIGFuZCBwb3J0aW9ucyB0aGVyZW9mIGFzIHNtYXJ0IGNvbnRyYWN0cyBvbiBibG9ja2NoYWluLWJhc2VkIGFwcGxpY2F0aW9ucyBhbmQgcHJvdG9jb2xzLiAgCiAgICBUaGUgVm9sdHogREFPIGlzIHBlcm1pdHRlZCB0byB1c2Ugc3ViY29udHJhY3RvcnMgdG8gZG8gdGhpcyB3b3JrLiAgCiAgICBUaGlzIGxpY2Vuc2UgaXMgY29uZGl0aW9uYWwgb24gVm9sdHogYW5kIHRoZSBWb2x0eiBEQU8gY29tcGx5aW5nIHdpdGggdGhlIHRlcm1zIG9mIHRoZSBCdXNpbmVzcyBTb3VyY2UgTGljZW5zZSAxLjEsIG1hZGUgYXZhaWxhYmxlIGF0IGh0dHBzOi8vZ2l0aHViLmNvbS9Vbmlzd2FwL3YzLWNvcmUvYmxvYi9tYWluL0xJQ0VOU0UuCiAgICA='
    }
  },
  raw: [
    {
      address: '0x4976fb03c32e5b8cfe2b6ccb31c09ba78ebaba41',
      key: '0x17c48fce163169c7aeef90819852e66530993e0ed99704e42b4017a04c2808cc',
      original: '0x0000000000000000000000000000000000000000000000000000000000000000',
      dirty: '0x00000000000000000000000000000000000000000000000000000000000007c1'
    }
  ]
}

this change is previously ignored due to original===null, also note that the raw object only report the first slot changed in a mapping so using the decoded field is crucial

Tested with no error with the ENS governor

[mds1]

Ah this makes sense. Since I can't dispatch workflows from branches from a fork (which makes sense, to avoid leaking secrets), I've pushed this commit to a temporary brach and kicked off the CI job

[gzeoneth]

thought on also implementing this? ArbitrumFoundation@cf816df
the CI job is spending so much time running old proposals that is already expired or executed

although that would also mean the CI would run nothing most of the time with this haha

[mds1]

Looks like this sims for this PR worked as expected! I noticed the Unsure if the same can happen to dirty, let's assume its possible to collect all keys comment, so I messaged @BogdanHabic and someone from Tenderly should be able to sanity check our state diff parsing here on Monday to make sure there's no other cases we might be handling incorrectly. So I'll wait for that before merging.

thought on also implementing this? ArbitrumFoundation@cf816df
the CI job is spending so much time running old proposals that is already expired or executed

although that would also mean the CI would run nothing most of the time with this haha

I agree this would be nice and the current CI is pretty wasteful. We have it tracked in #117, using the approach that the BDG Labs fork uses, to ensure old reports are cached and easily accessible.

The main reason it hasn't been done is that currently no one is actively working on improvements to Seatbelt. I'm no longer at ScopeLift, but previously we were maintaining it while I was there. I'm not sure if @apbendi and co. have any upcoming plans to keep working on some of the improvements being tracked in the issues

[gzeoneth]

I see, I might implement that later but for now let's merge this?

[beruda]

Hey @mds1 @gzeoneth 👋 Vuk from Tenderly here...

To me this looks good just off the top of my head, but I didn't manage to find the time to dive deep since we had too many support requests left over from the Holidays.

If you'd like a more certain answer, I can take a deeper look tomorrow and let you know. I understand that this is governance, so you'd like to be pretty certain, correct?

[mds1]

Thanks @beruda! A deeper look would be appreciated—I mainly just want to have confidence that the way we are parsing state diffs will not result in any missing or incorrect data :)

[strahor-13]

Hey @mds1 @gzeoneth , Aleksandar from Tenderly here 👋

I took a look at our code, and ran some tests. In short, your logic is correct.

If there is no change (i.e. original was changed to some value and then back to original) - we will either return no change, or the full change with original and dirty being identical (this depends on the type of the variable, as for some it is easier to check whether original == dirty than for others). However, we will not return null values in those cases.

The only time we will return null values is if we fail to decode the variable, as your comment on the other PR states.

In case of mappings, it is indeed possible that either original or dirty value for a certain key will be null. This mostly happens in cases where we fail to decode the variable. It is also possible to happen for some types when the value of a mapping was reset to 0-value. Regardless, your logic for checking if the keys are present in the map is the way to go for mappings.

This can also happen for elements of dynamic arrays, but I see that you are using raw state changes for them, so that will continue to work as expected.

Hope this clears things up for you!
If you have any more questions, please ping me. I'll be happy to help you!

[gzeoneth]

@mds1 can we merge or is there anything unresolved?

[mds1]

Sorry for the delay here, I was out most of the day today and didn't get to this yesterday.

Just caught up and we should be all set here, so going to merge this now. Thank you @beruda and @strahor-13 for the explanations and confirming our implementations, and thank you @gzeoneth for the PR!