Skip to content

Conversation

@szg251
Copy link
Collaborator

@szg251 szg251 commented Feb 3, 2022

Based on: #42

This PR introduces a way to delay the signing of transactions. This could be useful when we don't want to store the signing key(s) on the bot server. We still need their verification keys, in the same folder and in the same format as the signing keys, only with a vkey file extension.

@szg251 szg251 changed the base branch from master to gergely/wait-slots February 3, 2022 07:31
(map (encodeByteString . fromBuiltin . getPubKeyHash . Ledger.pubKeyHash) pubKeys)
]
printLog @w Warn (Text.unpack err)
pure $ Left err
Copy link
Contributor

Choose a reason for hiding this comment

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

For manual signing, should the transaction error?
As far as a contract resulting in a manual sign goes, it did its job correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, giving the tx here makes it pretty difficult to test contracts using this, i wonder if theres another way - perhaps a function specifically for under-signed transactions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bit unfinished, I just wanted to push it asap, but certainly I want to find a better way to handle this. Demanding manual intervention is pretty annoying, also if a contract contains multiple txs, the whole workflow could break.
I am also thinking about a way to inject a callback function to handle the unsigned tx. I'll try to sketch up something in the following days...

Copy link
Contributor

Choose a reason for hiding this comment

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

That could certainly work! Please do keep me in the loop on this :)

Comment on lines 179 to 184
let vKeyFiles =
map (\filename -> Text.unpack pabConf.pcSigningKeyFileDir ++ "/" ++ filename) $
filter ("vkey" `isExtensionOf`) files
let sKeyFiles =
map (\filename -> Text.unpack pabConf.pcSigningKeyFileDir ++ "/" ++ filename) $
filter ("skey" `isExtensionOf`) files
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like these could be combined in some way, are the files always either skey or vkey? if so, a partition could be used. either way, the map function could be applied before the filter to avoid writing it twice

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

case Map.lookup pkh privKeys of
Just privKey -> Right $ Tx.addSignature' privKey tx'
Just (FromSKey privKey) -> Right $ Tx.addSignature' privKey tx'
Just (FromVKey privKey) -> Right $ Tx.addSignature' privKey tx'
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 you use unDummyPrivateKey here over unpacking directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

skeyToPubKey =
Ledger.toPublicKey
. Files.unDummyPrivateKey
. fromRight (error "Impossible happened")
Copy link
Contributor

Choose a reason for hiding this comment

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

How impossible are we talking, maybe give something a little more meaningful, since getting "impossible happened" in an error would not be super helpful

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, sorry about that. Fixed it.

_
| "vkey" `isExtensionOf` filename -> Just <$> readVerificationKey @w fullPath
| "skey" `isExtensionOf` filename -> Just <$> readSigningKey @w fullPath
| otherwise -> pure Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Theres a function to extract the extension of a file, maybe case over that instead of a guards only case?

@szg251 szg251 marked this pull request as ready for review February 11, 2022 08:22
@szg251
Copy link
Collaborator Author

szg251 commented Feb 11, 2022

@samuelWilliams99 Are you okay with merging this as it is? I don't know when will i have the time to find a better way for handling unsigned txs, it might just be the tx export function, you're currently working on.

@szg251 szg251 force-pushed the gergely/manual-sign branch from f0e9139 to 8fe4ed0 Compare February 11, 2022 10:23
@samuelWilliams99
Copy link
Contributor

Sure! @nazrhom will be handling the get tx functionality, as per ticket #46

Base automatically changed from gergely/wait-slots to master February 11, 2022 12:18
@szg251 szg251 merged commit cd9bb41 into master Feb 11, 2022
@szg251 szg251 deleted the gergely/manual-sign branch February 11, 2022 12:18
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.

3 participants