Skip to content

Conversation

@stevenj
Copy link
Collaborator

@stevenj stevenj commented Aug 25, 2023

Catalyst toolbox was crashing if "purpose" was Null in a registration.
This fixes that, so Null is properly detected as a "Catalyst" registration.

Also, it adds the pre-calculated Jormungandr chain address, because its non trivial to go from the voting key to the chain address used by Jormungandr.

This lets me compare the SVE snapshot with the normal snapshot, and it shows the SVE snapshot has rounding errors which will negatively effect people with multiple registrations on the same key.

Recommendation is for Audit, we publish the updated snapshot artifact, not the one we import into vit-ss. And then make a quick program to convert the snapshot artifact into a format needed for vit-ss for F10. The utilities/snapshot-check/compare_snapshot.py would need a couple of tweaks to be such a tool.

@stevenj stevenj changed the title fix(catalyst-toolbox): Fix error when purpose is null. fix(catalyst-toolbox): Fix error when purpose is null. | NPG-0000 Aug 25, 2023
@stevenj stevenj requested a review from kukkok3 August 25, 2023 09:02
@stevenj stevenj enabled auto-merge (squash) August 25, 2023 09:02
kukkok3
kukkok3 previously approved these changes Aug 25, 2023
Copy link
Collaborator

@kukkok3 kukkok3 left a comment

Choose a reason for hiding this comment

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

LGTM

@minikin minikin added the review me PR is ready for review label Aug 25, 2023
@saibatizoku saibatizoku self-assigned this Aug 25, 2023
@stevenj stevenj dismissed minikin’s stale review August 26, 2023 05:16

Test are fragile, so while the suggested change is probably correct, the implementation tries to keep the original code as close as possible.

@stevenj stevenj disabled auto-merge August 26, 2023 05:18
@stevenj stevenj merged commit 3400229 into main Aug 26, 2023
@stevenj stevenj deleted the fix/catalyst-toolbox-snapshot-processor branch August 26, 2023 05:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review me PR is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants