-
Notifications
You must be signed in to change notification settings - Fork 217
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
Payment construction endpoint #2702
Conversation
, withdrawal :: !(Maybe ApiWithdrawalPostData) | ||
, metadata :: !(Maybe (ApiT TxMetadata)) | ||
, mint :: !(Maybe (ApiT W.TokenMap)) | ||
, delegation :: ![ApiMultiDelegationAction] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Anviking does it make sense? see ApiMultiDelegationAction
- Natural will stand for soft index of stake key. Thanks to that we could in one tx realize several join/leave pool actions. Maybe it could help with portfolio recreation, reduce needed roundtrips, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think [ApiMultiDelegationAction]
as a raw/manual/low-level kind of multi-delegation api would be really nice. Joining (ApiT PoolId) Natural
makes sense to me.
But instead of Leaving (ApiT PoolId)
, we should presumably have Leaving Natural
. We don't have to specify a pool id when we de-register a stake key, but we do need to know which stake key we want to deregister.
And I think we currently use the terminology Join
and Quit
, so why not stick to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I just realised I didn't plan for the DelegationState
to validate (several) user-provided multi-delegation actions, but it would be nice to support, and I don't see how it could be problematic.)
But it does mean that, as said, the delegation actions need to be validated by the DelegationState
eventually.
And for now, I guess you would have to start with only supporting key 0
or maybe even a single action, when implementing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the look. Now I have :
data ApiMultiDelegationAction = Joining (ApiT PoolId) Natural | Leaving Natural
997ac09
to
0d2184a
Compare
87efd4f
to
60fc62e
Compare
c5ebf05
to
fc1d4be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @paweljakubas
(notSupported "Byron") | ||
(notSupported "Shared") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps add a TODO: ADP-909
comment or something - These wallet types should be supported in the final version.
let toAddressAmount (ApiPaymentAddresses content) = | ||
addressAmountToTxOut <$> content | ||
toAddressAmount (ApiPaymentAll _) = undefined -- this will be tackled when migration is supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure our app doesn't implode when the user finds an implemented feature.
Can we use HTTP 501 instead of undefined
or error
, wherever possible?
let toAddressAmount (ApiPaymentAddresses content) = | |
addressAmountToTxOut <$> content | |
toAddressAmount (ApiPaymentAll _) = undefined -- this will be tackled when migration is supported | |
let toAddressAmount (ApiPaymentAddresses content) = pure (addressAmountToTxOut <$> content) | |
toAddressAmount (ApiPaymentAll _) = liftE ErrTemporarilyDisabled -- TODO: this will be tackled when migration is supported |
{ txWithdrawal = wdrl | ||
, txMetadata = md | ||
, txTimeToLive = ttl | ||
--, txDelegationAction -- this will be tackled when delegations are supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--, txDelegationAction -- this will be tackled when delegations are supported | |
, txDelegationAction = Nothing -- TODO: ADP-909 this will be tackled when delegations are supported |
pure (mkApiCoinSelection [] Nothing md sel', fee, blob) | ||
|
||
let txSerialized = ApiSerialisedTransaction (ApiBytesT $ convertToBase Base64 blob) | ||
pure $ ApiConstructTransaction txSerialized apiSelection (Quantity $ fromIntegral fee) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nicer if there were a separate:
mkApiConstructTransaction :: sel -> fee -> Either ErrMkTx ApiConstructTransaction
Currently W.constructUnsignedTransaction
is getting the reward account XPrv, but only the XPub is necessary for the TxBody, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, in the end XPub is needed.
, withdrawal :: !(Maybe ApiWithdrawalPostData) | ||
, metadata :: !(Maybe (ApiT TxMetadata)) | ||
, mint :: !(Maybe (ApiT W.TokenMap)) | ||
, delegations :: !(Maybe [ApiMultiDelegationAction]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget, why did we have Maybe [ApiMultiDelegationAction]
here?
Was there a semantic difference between Nothing
and Just []
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is reasonable to use delegations: Maybe ...
as it can be optional on Api level, one can omit "delegations" field.. I will use Maybe (NonEmpty ApiMultiDelegationAction)
though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from arguably looking a bit strange, wouldn't Maybe (NonEmpty ApiMultiDelegationAction)
forbid "delegations" = []
, potentially making it more difficult for folks to write API clients?
Could we not have delegations :: [ApiMultiDelegationAction]
, but have a custom JSON parser defaulting to []
if the key is not present?
... if we even need to allow it to be omitted?
, mkUnsignedTransaction | ||
:: AnyCardanoEra | ||
-- Era for which the transaction should be created. | ||
-> XPrv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only an XPub is needed here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, used XPub rather than XPrv
-> (Maybe Cardano.TxMetadata, [Cardano.Certificate]) | ||
-> SlotNo | ||
-- ^ Slot at which the transaction will expire. | ||
-> XPrv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> XPrv | |
-> RewardAccount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -1006,6 +1022,14 @@ x-transactionOutputs: &transactionOutputs | |||
amount: *amount | |||
assets: *walletAssets | |||
|
|||
x-transactionNoAmountOutputs: &transactionNoAmountOutputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't this extra declaration should be necessary.
It's just an array of addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it is used here :
2686 ApiPaymentDestination: &ApiPaymentDestination
2687 oneOf:
2688 - title: outputs with specified amounts
2689 <<: *transactionOutputs
2690 - title: outputs without amounts
2691 <<: *transactionNoAmountOutputs
and in contrast to transactionOutputs
it does not include amount
and assets
as we want to use it in migration. So how to capture migration output here?
specifications/api/swagger.yaml
Outdated
type: object | ||
required: | ||
- action | ||
- stake_key_index | ||
properties: | ||
action: | ||
type: string | ||
enum: ["quit", "join"] | ||
pool: *stakePoolId | ||
stake_key_index: *derivationSegment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of this way? I like it more.
type: object | |
required: | |
- action | |
- stake_key_index | |
properties: | |
action: | |
type: string | |
enum: ["quit", "join"] | |
pool: *stakePoolId | |
stake_key_index: *derivationSegment | |
oneOf: | |
- type: object | |
required: | |
- join | |
properties: | |
join: | |
type: object | |
required: | |
- pool | |
- stake_key_index | |
properties: | |
pool: *stakePoolId | |
stake_key_index: *derivationSegment | |
- type: object | |
required: | |
- quit | |
properties: | |
quit: | |
type: object | |
required: | |
- stake_key_index | |
properties: | |
stake_key_index: *derivationSegment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it is better. Adopted
required: | ||
- invalid_before | ||
- invalid_hereafter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would want the Haskell validity bound type to stay the same, but in JSON, the bounds should be optional, with a missing key meaning "unspecified".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we have at this moment :
2693 ApiConstructTransactionData: &ApiConstructTransactionData
2694 type: object
2695 required:
2696 - passphrase
2697 properties:
2698 passphrase:
2699 <<: *lenientPassphrase
2700 description: The wallet's master passphrase.
2701 payments: *ApiPaymentDestination
2702 withdrawal: *transactionWithdrawalRequestSelf
2703 metadata: *transactionMetadata
2704 mint: *transactionMint
2705 delegations: *transactionDelegations
2706 validity_interval: *ApiValidityInterval
so the idea was to sneak optionality by making validity_interval
optional, but make ApiValidityInterval
well defined. kinda the same as for ApiPaymentDestination
- the same idea...Once again I do not have strong opinion about this, could be either way, but maybe it would be ok to be consistent..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the JSON side, we want an omitted field to mean unspecified bound.
fc1d4be
to
590c35f
Compare
it "TRANS_NEW_CREATE_01x - Single Output Transaction" $ \ctx -> runResourceT $ do | ||
let initialAmt = 3*minUTxOValue | ||
wa <- fixtureWalletWith @n ctx [initialAmt] | ||
wb <- fixtureWalletWith @n ctx [initialAmt] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be emptyWallet
I guess.
specifications/api/swagger.yaml
Outdated
post: | ||
operationId: postTransactionConstruct | ||
tags: ["Transactions"] | ||
summary: Create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
summary: Create | |
summary: Construct |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work. I still have a few comments though, and have also pushed some minor changes.
lib/core/src/Cardano/Wallet.hs
Outdated
, selectionToUnsignedTx | ||
, signTransaction | ||
, constructUnsignedTransaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just call this constructTransaction
lib/core/src/Cardano/Wallet.hs
Outdated
constructUnsignedTransaction ctx wid mkRwdAcct pwd txCtx sel = | ||
db & \DBLayer{..} -> do | ||
era <- liftIO $ currentNodeEra nl | ||
withRootKey @_ @s ctx wid pwd ErrConstructTxWithRootKey $ \xprv scheme -> do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we aren't signing anything then we shouldn't need the wallet root key.
We should be able to get rewardAcct
from the database using readRewardAccount
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -323,6 +331,7 @@ byronTransactionClient = | |||
, postExternalTransaction = _postExternalTransaction | |||
, deleteTransaction = _deleteTransaction | |||
, getTransaction = _getTransaction | |||
, constructTransaction = error "TODO should be supported in the final version of Transaction Workflow." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, constructTransaction = error "TODO should be supported in the final version of Transaction Workflow." | |
, constructTransaction = error "TODO ADP-909 implement new transaction API for all wallet types." |
@@ -633,6 +634,21 @@ getTransaction w t = discriminate @style | |||
tid = ApiTxId (t ^. typed @(ApiT (Hash "Tx"))) | |||
mkURL mk = mk wid tid | |||
|
|||
createUnsignedTransaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createUnsignedTransaction | |
createTransaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, there is already createTransaction
toJSON = genericToJSON defaultRecordTypeOptions | ||
|
||
instance ToJSON ApiValidityBound where | ||
toJSON ApiValidityBoundUnspecified = String "unspecified" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use Null
I think.
(Just joinObj, Nothing) -> do | ||
poolId <- joinObj .: "pool" | ||
ix <- joinObj .: "stake_key_index" | ||
pure $ Joining poolId ix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just joinObj, Nothing) -> do | |
poolId <- joinObj .: "pool" | |
ix <- joinObj .: "stake_key_index" | |
pure $ Joining poolId ix | |
(Just o, Nothing) -> Joining <$> o .: "pool" <*> o .: "stake_key_index" |
(Nothing, Just quitObj) -> do | ||
ix <- quitObj .: "stake_key_index" | ||
pure $ Leaving ix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nothing, Just quitObj) -> do | |
ix <- quitObj .: "stake_key_index" | |
pure $ Leaving ix | |
(Nothing, Just o) -> Leaving <$> o .: "stake_key_index" |
(Nothing, Just quitObj) -> do | ||
ix <- quitObj .: "stake_key_index" | ||
pure $ Leaving ix | ||
_ -> fail "ApiMultiDelegationAction needs 'join' or 'quit' field" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ -> fail "ApiMultiDelegationAction needs 'join' or 'quit' field" | |
_ -> fail "ApiMultiDelegationAction needs either 'join' or 'quit', but not both" |
pure ApiValidityBoundUnspecified | ||
else | ||
fail "invalid string of ApiValidityBound" | ||
processObject = withObject "ApiValidityBound object" $ \o -> do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible try parsing the Quantities directly without testing the "unit" field.
required: | ||
- invalid_before | ||
- invalid_hereafter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the JSON side, we want an omitted field to mean unspecified bound.
Currently in swagger only Anyway, one can currently set just
It seems that one full utxo is selected and "spent" to one of the wallet's own addresses... |
In the case where the user didn't provide any of payments, delegations or mint, I'm not sure whether we need to disallow it - as long as the result is a valid transaction. Perhaps a user would like to post a transaction with nothing but metadata? |
That would also incur fees + minUtxoValue, so in fact (at least) a payment, right? |
2df0c15
to
b27561a
Compare
bors r+ |
2702: Payment construction endpoint r=paweljakubas a=paweljakubas # Issue Number <!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. --> adp-909 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] POST transactions/construct in swagger - [x] Api.Types reflecting things added in swagger - [x] add Api skeleton - [x] unit tests for Api.Types, Malformed - [x] extend Transaction interface by adding mkUnsignedTransaction - [x] fill in implementation of Cardano.Api.Server and Cardano-Wallet function implementing 'payments' functionality - [x] prepare first integration testing # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed:
|
bors retry |
2702: Payment construction endpoint r=paweljakubas a=paweljakubas # Issue Number <!-- Put here a reference to the issue that this PR relates to and which requirements it tackles. Jira issues of the form ADP- will be auto-linked. --> adp-909 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] POST transactions/construct in swagger - [x] Api.Types reflecting things added in swagger - [x] add Api skeleton - [x] unit tests for Api.Types, Malformed - [x] extend Transaction interface by adding mkUnsignedTransaction - [x] fill in implementation of Cardano.Api.Server and Cardano-Wallet function implementing 'payments' functionality - [x] prepare first integration testing # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed:
|
bors retry |
Build succeeded: |
Issue Number
adp-909
Overview
Comments