Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Fix LIP0054 open issues #285

Merged
merged 10 commits into from
Mar 10, 2023
Merged

Fix LIP0054 open issues #285

merged 10 commits into from
Mar 10, 2023

Conversation

ricott1
Copy link
Contributor

@ricott1 ricott1 commented Feb 27, 2023

Closes #283.

  • Address internal review.
  • Add some extra checks to the message recovery and state recovery commands.
  • Fixes createTerminatedStateAccount function to actually make use of the input stateRoot

@ricott1 ricott1 self-assigned this Feb 27, 2023
proposals/lip-0054.md Outdated Show resolved Hide resolved
proposals/lip-0054.md Outdated Show resolved Hide resolved
proposals/lip-0054.md Outdated Show resolved Hide resolved
if terminatedOutboxAccount(trsParams.chainID) exists:
raise Exception("Terminated outbox account already exists.")

queryKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_CHANNEL_DATA + sha256(getMainchainID())
queryKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_CHANNEL_DATA + sha256(OWN_CHAIN_ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW: we should make sure that we have a unit test where this verify function is called where OWN_CHAIN_ID != getMainchainID(). Otherwise, I'd be a bit afraid that the implementation would not be changed as sha256(getMainchainID()) should work for all other SKD 6 tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

good point, I have noted it down in the issue (LiskArchive/lisk-sdk#8254) on SDK

proposals/lip-0054.md Show resolved Hide resolved
proposals/lip-0054.md Outdated Show resolved Hide resolved
Co-authored-by: AndreasKendziorra <[email protected]>
Copy link
Contributor

@gkoumout gkoumout left a comment

Choose a reason for hiding this comment

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

Looks good to me, left few minor suggestions and comments and some questions to make sure we are in the same page.

proposals/lip-0045.md Show resolved Hide resolved
proposals/lip-0054.md Show resolved Hide resolved
proposals/lip-0054.md Outdated Show resolved Hide resolved
proposals/lip-0054.md Show resolved Hide resolved
proposals/lip-0054.md Outdated Show resolved Hide resolved
proposals/lip-0054.md Outdated Show resolved Hide resolved
proposals/lip-0054.md Show resolved Hide resolved
proposals/lip-0054.md Outdated Show resolved Hide resolved
proposals/lip-0054.md Show resolved Hide resolved
proposals/lip-0045.md Outdated Show resolved Hide resolved
@ricott1 ricott1 requested a review from gkoumout March 9, 2023 14:56
Copy link
Contributor

@gkoumout gkoumout left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@ishantiw ishantiw left a comment

Choose a reason for hiding this comment

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

proposals/lip-0054.md Outdated Show resolved Hide resolved
if terminatedOutboxAccount(trsParams.chainID) exists:
raise Exception("Terminated outbox account already exists.")

queryKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_CHANNEL_DATA + sha256(getMainchainID())
queryKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_CHANNEL_DATA + sha256(OWN_CHAIN_ID)
Copy link
Contributor

Choose a reason for hiding this comment

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

good point, I have noted it down in the issue (LiskArchive/lisk-sdk#8254) on SDK

@ricott1 ricott1 requested a review from janhack March 9, 2023 17:40
Copy link
Contributor

@janhack janhack left a comment

Choose a reason for hiding this comment

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

Editorial approval

@janhack janhack merged commit 6796693 into main Mar 10, 2023
@janhack janhack deleted the ale_address_review_lip54 branch March 10, 2023 09:08
@@ -161,7 +180,8 @@ stateRecoveryParams = {
"siblingHashes": {
"type": "array",
"items": {
"dataType": "bytes"
"dataType": "bytes",
"length": HASH_LENGTH
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small inconsistency but here you use 'length' but in the other params you are using 'minLength' and 'maxLength'.

# Check that all query keys are pariwise distinct,
# meaning that we are not trying to recover the same entry twice.
if len(queryKeys) != len(set(queryKeys)):
raise Exception("Recovered store keys are not pairwise distinct.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just say 'distinct' for simplicity

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LIP0054: fix inclusion proof query keys
6 participants