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

Add Mock option to Tx Building #11

Merged
merged 7 commits into from
Oct 13, 2022

Conversation

nothingalike
Copy link
Member

No description provided.

@nothingalike nothingalike marked this pull request as draft July 7, 2022 20:04
@nothingalike
Copy link
Member Author

Working on this update (c89b2ae)

I realized we should come up with a standard on how we manipulate transactions and how we expect further transaction updates to occur.

Version 2.10.1 of CardanoSharp.Wallet introduced a new TransactionWitnessSetBuilder action to clear mocked vkey witnesses, but cscli's transaction implementation leverages another CardanoSharp.Wallet feature which will calculate and set fee in one method call.

Both of these functions are useful, but not together. Now I'm not sure if we should just pick a stance on how transactions should be built, or if we should always add functionality that achieves the same goal but can be used regardless if you use builders or not. Example: I could added an extension for TransactionWitnessSet which would remove mocked just like the builder.

@safestak-keith

@@ -90,6 +103,17 @@ public async ValueTask<CommandResult> ExecuteAsync(CancellationToken ct)
// Fee Calculation
var fee = tx.CalculateAndSetFee(protocolParams.MinFeeA, protocolParams.MinFeeB);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I suggest a mock witness count parameter in CalculateFee and CalculateAndSetFee (with a default value of 0) again? Transactions will more likely be built in a separate environment to where the signing/witnessing will happen so it makes sense for CardanoSharp.Wallet to cater for this scenario instead of having all consumers replicate this code?

@safestak-keith
Copy link
Collaborator

Working on this update (c89b2ae)

I realized we should come up with a standard on how we manipulate transactions and how we expect further transaction updates to occur.

My simple view is that since the transaction body and metadata is generally finalised by one party, the only part of a transaction that should be manipulated is the witness set. Changing anything else will change the signature payload which invalidates the previous witnesses. Given the the typical transaction life cycle:

  1. Build and finalise transaction body and auxiliary data
  2. Calculate fees based on expected witnesses
  3. Sign transaction (repeat if required for multisig or complex transactions)
  4. Submit transaction

Steps 1 and 2 would happen in the same context in the majority of cases, but steps 3 and 4 can cross process/network boundaries and switch hands with some level of coordination. However that coordination should entirely be an application layer concern or handled manually out-of-band (e.g. a manual process in discord, a Orleans-based app which orchestrates signatures, etc.).

So with that in mind I'm definitely leaning towards the cardano-cli approach of finalising the body+metadata, calculating the fees based on the expected number of witnesses but leaving the witnesses empty, and leaving it to the layers above to coordinate the signatures and append their witnesses one by one.

@nothingalike
Copy link
Member Author

@safestak-keith

Ok yeah that definitely makes sense for general flow.

Do you think we should empty out the Mock Witnesses after calculating the fee (maybe inside the new Calculate method) or should i just create an extension specifically to remove the mocks?

@safestak-keith
Copy link
Collaborator

@safestak-keith

Ok yeah that definitely makes sense for general flow.

Do you think we should empty out the Mock Witnesses after calculating the fee (maybe inside the new Calculate method) or should i just create an extension specifically to remove the mocks?

The first option makes sense to me, unless you can think of a scenario where consumers might want to do explicitly create/remove them?

@nothingalike
Copy link
Member Author

@safestak-keith

This PR: CardanoSharp/cardanosharp-wallet#88 is in response to this comment: #11 (comment)

@nothingalike nothingalike marked this pull request as ready for review July 23, 2022 00:22
@safestak-keith
Copy link
Collaborator

LGTM

@safestak-keith safestak-keith merged commit c282432 into main Oct 13, 2022
@safestak-keith safestak-keith deleted the feature/tx-building-mock-witnesses branch October 13, 2022 00:02
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