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

Refactor signTx #4128

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Refactor signTx #4128

wants to merge 15 commits into from

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Sep 19, 2023

  • Handle normal key wits
  • Handle key wits from simple scripts
  • Handle byron/bootstrap wits
  • Polish
    • Don't rely on mock addresses
      • Redefine isOurs for SeqState taking KeyFingerprint instead (basically a KeyHash) -> NO
      • Figure out how to make the script lookup / signing more proper 🤔
    • Redefine estimateKeyWitnessCount
  • Testing
    • Integration tests green
      • Fix TRANS_NEW_ASSETS_CREATE_02 - using reference script
    • Add unit and property tests

Comments

Issue Number

ADP-2675

@Anviking Anviking self-assigned this Sep 19, 2023
$ Cardano.InAnyCardanoEra era
$ Write.toCardanoTx
$ f eraForSigning
$ Write.fromCardanoTx ctx
Copy link
Member Author

@Anviking Anviking Sep 19, 2023

Choose a reason for hiding this comment

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

Should possibly take a value level RecentEra era instead of the constraint

Comment on lines +338 to +344
-- FIXME: HACK because shared wallets have 'ScriptHash -> addrXPrv' lookup,
-- not 'KeyHash -> addrXPrv' lookup.
timelockHashes = Set.fromList $ map (fromCAScriptHash . CA.toScriptHash) timelockScripts
where
fromCAScriptHash = fromMaybe err . keyHashFromBytes . CA.unScriptHash
where
err = error "fromCAKeyHash invalid hash"
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to solve

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably we can just add a separate resolver without downside

Problem is that 'mkExtraWitness' isn't signed by keyLookup anymore.

mkExtraWitness :: Cardano.Hash Cardano.PaymentKey -> Maybe (Cardano.KeyWitness era)
mkExtraWitness vkh = do
    -- NOTE: We cannot resolve key hashes directly, so create a one-time
    -- temporary address with that key hash which is fine to lookup via the
    -- address lookup provided above. It works _fine_ because the discovery
    -- of addresses is done properly based on the address constituents (i.e.
    -- the key hash) and not the overall address itself.
    let addr = Cardano.makeShelleyAddress networkId
            (Cardano.PaymentCredentialByKey vkh)
            Cardano.NoStakeAddress
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.

1 participant