Skip to content

Conversation

@mkysel
Copy link
Collaborator

@mkysel mkysel commented Oct 16, 2025

Upgrade local chain and test Anvil containers to contracts v0.6.0 and update generated ABI bindings, events, and parameter parsing across blockchain and indexer code

Align the repo with contracts v0.6.0, updating generated bindings for event signatures and topics, adjusting parameter parsing to return a string key, and revising payer report and registry logic to include uint24 nonce and bytes32 payerReportId while switching node ID hashing to ABI-encoded uint32[] with keccak256. Docker and Anvil images move to ghcr.io/xmtp/contracts:v0.6.0, and anvil environment addresses refresh.

📍Where to Start

Start with the generated bindings changes and their downstream usage in payerregistry and parameter registry parsing, then follow into blockchain adapters and tests: pkg/abi/payerregistry/PayerRegistry.go, pkg/blockchain/parameter_registry_admin.go, and pkg/utils/hash.go.


📊 Macroscope summarized 10c738e. 7 files reviewed, 1 issue evaluated, 1 issue filtered, 0 comments posted

🗂️ Filtered Issues

pkg/utils/hash.go — 0 comments posted, 1 evaluated, 1 filtered
  • line 63: PackSortAndHashNodeIDs now feeds args.Pack(nodeIDs) (i.e. full abi.encode for a dynamic uint32[]) into the hash. ABI encoding a dynamic array prepends the head/offset and the element count before the element words. The previous implementation hashed only the concatenation of the 32-byte, left-padded element words (equivalent to abi.encodePacked of the array). As a result, every call with a non-empty activeNodeIDs slice now produces a different activeNodeIdsHash, and therefore BuildPayerReportID returns a different ReportID for the same report payload. Any previously stored or on-chain report IDs will no longer match what the node computes after this change, breaking report verification/lookup. Example: for []uint32{1}, the old code hashed keccak256(0…01), while the new code hashes keccak256( offset || length || 0…01 ), yielding a different digest. This is a breaking runtime regression affecting every caller. [ Low confidence ]

@mkysel mkysel requested a review from a team as a code owner October 16, 2025 19:11
@graphite-app
Copy link

graphite-app bot commented Oct 16, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • Queue - adds this PR to the back of the merge queue
  • Hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@mkysel mkysel requested review from fbac and neekolas October 20, 2025 14:06
Copy link
Contributor

@neekolas neekolas left a comment

Choose a reason for hiding this comment

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

The real test is whether the integration tests still pass. The changes themselves LGTM if the contracts agree

@mkysel mkysel force-pushed the mkysel/contracts-v0.6.0 branch from cf868ea to 10c738e Compare October 28, 2025 21:25
@mkysel mkysel merged commit 0518d20 into main Oct 28, 2025
11 checks passed
@mkysel mkysel deleted the mkysel/contracts-v0.6.0 branch October 28, 2025 23:13
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.

4 participants