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

fix: ignored state change #124

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Conversation

gzeoneth
Copy link
Contributor

Tenderly no longer return diff.original === null for storage slot that went from original -> other value -> original, but instead will have diff.original === null for un-decoded slot. Removing the ignore to make sure seatbelt surface those storage changes.

Copy link
Contributor

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

Thank you!

@mds1 mds1 merged commit 1ea60e4 into Uniswap:main Feb 14, 2024
2 checks passed
@mds1
Copy link
Contributor

mds1 commented Feb 14, 2024

Hm looks like this might still be needed? Seeing errors in CI after merging:

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at file:///home/runner/work/governance-***/governance-***/checks/check-state-changes.ts:87:31
    at Array.forEach (<anonymous>)
    at Object.checkProposal (file:///home/runner/work/governance-***/governance-***/checks/check-state-changes.ts:69:13)
    at file:///home/runner/work/governance-***/governance-***/index.ts:113:47
    at Array.map (<anonymous>)
    at main (file:///home/runner/work/governance-***/governance-***/index.ts:109:33)

@gzeoneth
Copy link
Contributor Author

Hm looks like this might still be needed? Seeing errors in CI after merging:

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at file:///home/runner/work/governance-***/governance-***/checks/check-state-changes.ts:87:31
    at Array.forEach (<anonymous>)
    at Object.checkProposal (file:///home/runner/work/governance-***/governance-***/checks/check-state-changes.ts:69:13)
    at file:///home/runner/work/governance-***/governance-***/index.ts:113:47
    at Array.map (<anonymous>)
    at main (file:///home/runner/work/governance-***/governance-***/index.ts:109:33)

hmm might have to make it conditional, I will check and make a new pr if needed

mds1 added a commit that referenced this pull request Feb 15, 2024
mds1 added a commit that referenced this pull request Feb 15, 2024
@mds1
Copy link
Contributor

mds1 commented Feb 15, 2024

Thanks, reverted for now with #125

@gzeoneth
Copy link
Contributor Author

gzeoneth commented Feb 15, 2024

there is more to that, in short the current check is missing many state change

{
  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'
    }
  ]
}

here is one of the failing case, note that original==null while state actually changed, another thing to note is that the raw array also doesn't give the full change, related to this todo

          // TODO arrays and nested mapping are currently not well supported -- find a transaction
          // that changes state of these types to inspect the Tenderly simulation response and
          // handle it accordingly. In the meantime we show the raw state changes and print a
          // warning about decoding the data

edit: another failing case

{
  address: '0x30200e0cb040f38e474e53ef437c95a1be723b2b',
  soltype: {
    name: 'texts',
    type: 'mapping (bytes32 => mapping (string => string))',
    storage_location: 'storage',
    offset: 0,
    index: '0x0000000000000000000000000000000000000000000000000000000000000007',
    indexed: false
  },
  original: null,
  dirty: {
    '0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae': { oracle: 'ZXhwb25lbnRpYWw=' }
  },
  raw: [
    {
      address: '0x30200e0cb040f38e474e53ef437c95a1be723b2b',
      key: '0xe9214943b30449a34fb13151c847d4d46653d9d822190f380b1dc193fff95b09',
      original: '0x0000000000000000000000000000000000000000000000000000000000000000',
      dirty: '0x6578706f6e656e7469616c000000000000000000000000000000000000000016'
    }
  ]
}

@gzeoneth
Copy link
Contributor Author

Should be fixed in #126

gzeoneth added a commit to ArbitrumFoundation/governance-seatbelt that referenced this pull request Mar 1, 2024
* fix: ignored state change (Uniswap#124)

* Revert "fix: ignored state change (Uniswap#124)" (Uniswap#125)

This reverts commit 1ea60e4.

* feat: only pending mode

* feat: new report id scheme

* fix: add 1 for alphanumeric ordering

* feat: store reports in separate directory

* fix: retryable reports title

* fix: retryable reports title

* fix: check state changes

* chore: change to ONLY_RELEVANT

* fix: ci env

* ci: try fix artifact upload

* debug: ci list reports

* revert: "debug: ci list reports"

This reverts commit 1d6833c.

* revert: "ci: try fix artifact upload"

This reverts commit 3fb7827.

* fix: governorAddress for crosschain sim

* fix: state diff is empty

---------

Co-authored-by: Matt Solomon <matt@mattsolomon.dev>
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