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

feat(cardano): validate pool registration owner witness path #74

Open
wants to merge 5 commits into
base: cardano-multisig
Choose a base branch
from

Conversation

davidmisiak
Copy link
Collaborator

Adds validation of witness request path against the path in the pool registration certificate, as proposed by @janmazak.

Copy link
Collaborator

@gabrielKerekes gabrielKerekes left a comment

Choose a reason for hiding this comment

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

I'm not sure we actually need the PoolOwnerPathChecker.

The owner and witness paths are already checked in the AccountPathChecker. Yes, it only compares accounts, but that's enough for staking paths as they have to end with /2/0 (given by SCHEMA_STAKING_ANY_ACCOUNT), and the paths have to follow the schema in both the updated code and in the old code too. So a different staking path would fail in account path checker.

IMHO the only thing we need to add on top of the old code would be validating that there is only one witness request in _validate_stake_pool_registration_tx_structure.

Or am I missing something?

Comment on lines +15 to +16
INVALID_STAKEPOOL_REGISTRATION_TX_WITNESS = wire.ProcessError(
"Stakepool registration transaction can only contain sinlge staking witnesses with the same path as in the certificate"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
INVALID_STAKEPOOL_REGISTRATION_TX_WITNESS = wire.ProcessError(
"Stakepool registration transaction can only contain sinlge staking witnesses with the same path as in the certificate"
INVALID_STAKEPOOL_REGISTRATION_TX_WITNESS = wire.ProcessError(
"Stakepool registration transaction can only contain a single staking witness and its path must match the certificate owner's path"

}
},
{
"description": "Sample stake pool registration certificate with additional witness request with different staking path",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test fails because the witness path is not a valid staking path not because it's different from the pool owner path.

@davidmisiak
Copy link
Collaborator Author

Note for future readers: Currently different staking paths aren’t allowed because of the single-account policy and there being only one staking key per account. However when CIP-0018 should be supported on HW wallets this is the implementation for the pool owner witness request path validation.

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