Payment Transaction Improvements #59
Conversation
|
Thanks a lot @nemo83 . Great enhancement. Just had a quick look. I will go through the PR in detail tomorrow. "plus" sounds ok to me. Yes, let's add some tests. You may also want to add an integration test. Also, you may need to do a pull to update your branch. I just noticed "cardano-serialization-lib" sub module is still there in your fork. That dependency has already been removed. |
|
Thanks for the quick response.
Gave that a go and I'm getting a value not conservative .
I think the problem is that fee at payment transaction object does not make
sense anymore now, but should be an independent parameter.
Will rebase from upstream
…On Wed 5 Jan 2022, 16:02 Satya, ***@***.***> wrote:
Thanks a lot @nemo83 <https://github.com/nemo83> . Great enhancement.
Just had a quick look. I will go through the PR in detail tomorrow.
"plus" sounds ok to me.
Yes, let's add some tests. You may also want to add an integration test.
Also, you may need to do a pull to update your branch. I just noticed
"cardano-serialization-lib" sub module is still there in your fork. That
dependency has already been removed.
—
Reply to this email directly, view it on GitHub
<#59 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFLRDWGEV3BKRMJTY3E53LUURTQDANCNFSM5LJ43W3Q>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Thanks @nemo83
In case of multiple PaymentTransactions, you need to set the fee only in one transaction. Otherwise n * fee will be paid. Here's a test case for your reference :
Agreed. We can plan to deprecate that and remove it later.
We can explore if somehow we can pass a flag "consolidateOutputs" or "combineOutputValue" ... and then call this optimization method if the flag is set. One option, we can add this flag to "TransactionDetailsParams" Let me know your thoughts. |
I still haven't figured out the Non Conservative issue in the PR 😭 |
Yes, a separate PR makes sense.
You are right. I was overthinking. If someone wants more control, then low level serialization api is there. To keep it simple, high level api can behave in a certain way. |
| @@ -1,11 +1,28 @@ | |||
| package com.bloxbean.cardano.client.util; | |||
|
|
|||
| import java.util.Objects; | |||
There was a problem hiding this comment.
@satran004 what's the best way of fixing this?
I need equals and hashcode for the grouping to work properly. Should we introdue lombok instead?
Or leave it as is and create a test with the description so that if they get deleted the test will remind why we need them?
There was a problem hiding this comment.
@nemo83 We can introduce Lombak if that solves your problem. We are using Lombok in other places.
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
...ain/java/com/bloxbean/cardano/client/backend/api/helper/impl/UtxoTransactionBuilderImpl.java
Outdated
Show resolved
Hide resolved
|
|
||
| if(minCost != null && totalLoveLace.compareTo(minCost) != 1) { | ||
| if (minCost != null && totalLoveLace.compareTo(minCost) != 1) { |
There was a problem hiding this comment.
CompareToZero: The result of #compareTo or #compare should only be compared to 0. It is an implementation detail whether a given type returns strictly the values {-1, 0, +1} or others. (details)
(at-me in a reply with help or ignore)
| * @param totalFee | ||
| * @param transactionOutputs | ||
| * @param senderMiscCostMap | ||
| * @param protocolParams |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| * @param detailsParams | ||
| * @param totalFee | ||
| * @param transactionOutputs | ||
| * @param senderMiscCostMap |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| * Deprecated, see createReceiverOutputsAndPopulateCostV2 | ||
| * @param transaction | ||
| * @param detailsParams | ||
| * @param totalFee |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
|
|
||
| /** | ||
| * Deprecated, see createReceiverOutputsAndPopulateCostV2 | ||
| * @param transaction |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| * @param transaction | ||
| * @param detailsParams | ||
| * @param totalFee | ||
| * @param transactionOutputs |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
|
|
||
| /** | ||
| * returns a new asset that is the sum of this and that (asset passed as parameter) | ||
| * @param that |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| /** | ||
| * Creates a new list of multi assets from those passed as parameters. | ||
| * Multi Assets with the same policy id will be aggregated together, and matching assets summed. | ||
| * @param multiAssets1 |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| * Creates a new list of multi assets from those passed as parameters. | ||
| * Multi Assets with the same policy id will be aggregated together, and matching assets summed. | ||
| * @param multiAssets1 | ||
| * @param multiAssets2 |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
|
|
||
| /** | ||
| * Sums a Multi Asset to another. If an Asset is already present, sums the amounts. | ||
| * @param that |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| /** | ||
| * Sums a Multi Asset to another. If an Asset is already present, sums the amounts. | ||
| * @param that | ||
| * @return |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| @@ -65,4 +65,15 @@ public static Value deserialize(Array valueArray) { | |||
| return value; | |||
| } | |||
|
|
|||
| /** | |||
| * Sums arbitrary complex values. | |||
| * @param that | |||
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| /** | ||
| * Sums arbitrary complex values. | ||
| * @param that | ||
| * @return |
There was a problem hiding this comment.
EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description. (details)
(at-me in a reply with help or ignore)
| public T _1; | ||
| public Z _2; | ||
|
|
||
| public Tuple(T _1, Z _2) { | ||
| this._1 = _1; | ||
| this._2 = _2; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { |
There was a problem hiding this comment.
EqualsGetClass: Prefer instanceof to getClass when implementing Object#equals. (details)
(at-me in a reply with help or ignore)
| * @param protocolParams | ||
| * @return | ||
| */ | ||
| @Deprecated | ||
| private BigInteger createReceiverOutputsAndPopulateCost(PaymentTransaction transaction, TransactionDetailsParams detailsParams, BigInteger totalFee, |
There was a problem hiding this comment.
UnusedMethod: Private method 'createReceiverOutputsAndPopulateCost' is never used. (details)
(at-me in a reply with help or ignore)
| @@ -378,6 +382,82 @@ private void checkAndAddAdditionalUtxosIfMinCostIsNotMet(Map<String, Set<Utxo>> | |||
| return senderToUtxoMap; | |||
| } | |||
|
|
|||
| private BigInteger createReceiverOutputsAndPopulateCostV2(List<PaymentTransaction> transactions, TransactionDetailsParams detailsParams, BigInteger totalFee, | |||
There was a problem hiding this comment.
UnusedVariable: The parameter 'detailsParams' is never read. (details)
(at-me in a reply with help or ignore)
|
@satran004 struggling to run tests on Mac Book Pro x86_64. I've run the I get this error on: |
@nemo83 I tried on mac x86 with your repo. It's working for me though 3 tests are failing but different error. Can you please delete native, rust/target folders under root folder and try to build rust module again ? |
Thanks for the reply, you pointed me in the right direction, not a rust expert, but a After cleaning the folders you mentioned, I also fixed all the tests, updating them in line with the rationale of the test, I've added comments in tests for clarity. I hope I will find time for some additional testing and a couple of integration tests. I'm overall happy with the outcome so far. |
| @@ -85,9 +88,9 @@ dependencies = [ | |||
|
|
|||
| [[package]] | |||
| name = "ed25519" | |||
| version = "1.3.0" | |||
| version = "1.2.0" | |||
There was a problem hiding this comment.
@satran004 this is what was bombing out. I needed a cargo update to generate the lib
src/test/java/com/bloxbean/cardano/client/backend/api/helper/FeeCalculationServiceTest.java
Show resolved
Hide resolved
@nemo83 Great progress. Let me know once it's ready for merge. Fyi, To run integration tests in IDE, you can set the project id in this class BFBaseTest.java. |
I've added just one IT as unit tests do already a pretty good job for the type of change I did. I think the PR is ready to be merged if no objections or issue in the review? |
|
Also for some reasons the contract IT are failing on branch as well as aon master. so I geuss it's a problem with the tests? |
| existingMiscCost = existingMiscCost.add(costs); | ||
|
|
||
| // Update costs per (sender) base address | ||
| senderMiscCostMap.put(entry.getKey()._1.baseAddress(), existingMiscCost); |
There was a problem hiding this comment.
@nemo83, thanks for the optimization.
Are we missing datumHash in case of transfer to a script address ? So if there is a datumHash set in PaymentTransaction, it should be added to the output.
I am just thinking what would happen when there are two PaymentTransactions (to the same script address), with two different datum hash. Should we also group PaymentTransaction based on datumHash (sender, receiver, datumHash) ?
Here's the link to existing code
We can merge this PR and create an issue to track that as a separate PR.
Please let me know.
There was a problem hiding this comment.
Or, ignore grouping in case there is datumhash. Probably a safer option.
There was a problem hiding this comment.
That's a great catch. I guess I'm currently grouping for (sender, receiver), but because the datum hash is like another dimension of the transaction, I think I need to refactor the code to take that one in account too.
WDYT ?
There was a problem hiding this comment.
@nemo83 Is it possible to ignore grouping when datumhash is there and just consider it as a separate output ? In case of script output, I am not sure if we can add this intelligence. It depends on the logic in validator script and output becomes non spendable based on the logic.
Probably safe to keep these output separate.
BTW, I just tried contract IT tests. Those are failing because of missing datumHash. The contract call transaction doesn't find any utxo with datum hash as the previous payment transaction ignores datum hash in output :)
There was a problem hiding this comment.
Gotcha, it makes sense tests were failing. Of course the DatumHash wasnt set.
I'm gonna propose some changes. I believe grouping can still be safely done.
There was a problem hiding this comment.
Right I've suggested some changed for the datum hash.
Pleasee check code here.
Although all the smart contracts tests are now passing, the following is constantly failing, in both master and this branch
TransactionHelperServiceIT. mintTokenWithNormalAccountKeyHash()
I will try to take a look at it tomorrow.
There was a problem hiding this comment.
@nemo83 Please ignore the test failure for now. I think it is due to another check-in. I will fix it in the master after merge.
@nemo83 Yes. There is an issue with collateral amount for one of the contract. I need to increase the amount. I will fix that after merge. |
…d MultiAssets on the same utxo
| @Data | ||
| @AllArgsConstructor | ||
| private class PaymentTransactionGroupingKey { | ||
| private Account sender; | ||
| private String receiver; | ||
| private String datumHash; | ||
| } | ||
|
|
There was a problem hiding this comment.
This case class identifies the transaction grouping rationale. Basically in order to collate transactions in just one input:
- sender
- receiver
- datum (hash)
need to be the same.
|
@nemo83 Merging this PR. It looks good to me. Thanks a lot for this enhancement. Less fee is always better :) |
|
Thanks for the hard work. It's only my pleasure being able to contribute to a good opensource project. Even more if it's for cardano! |
This PR aims to collate all payments towards the same address together.
plus(happy to get renamed toaddor whatever makes sense) to theAsset,MultiAssetandValueclassescreateReceiverOutputsAndPopulateCostV2method, this should/wil manage all the tx together andmergethese going to the same address.This should save in fees thanks to the creation of a single Value from the multiple Value objects creaated from a PAymentTransaction object.
It should also allow to solve the problem of this tx: https://cardanoscan.io/transaction/61f7f2d030b90344f9e3e6a0fabdfbedb13abc94ba33bc3453e6604fcdfd7c7f
Where I want to send a specific amount of ada (greated than the minAda) along with a native asset. In the tx above, despite having sent enough ada to the recipient address, I've still paid the min ada to send along with the native asset.
I haven't tested the PR yet, so please bear with me :-D