Split policy ID assets when overflow #236
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
before adding the fix for this error, we first make add test with a situation which leads to the exact error we are expecting, in this case the "NFTs too large for change output" error
…y-id-assets-in-overflow # Conflicts: # rust/src/tx_builder.rs
|
@pedromtcosta , please resolve conflicts |
pedromtcosta
force-pushed
the
feature/split-policy-id-assets-in-overflow
branch
from
b41526e
to
17b02f1
Compare
|
@vsubhuman Conflicts resolved |
…y-id-assets-in-overflow # Conflicts: # rust/src/tx_builder.rs
|
@ozgrakkurt plz review |
|
@rooooooooob , check plz if you have a chance |
rooooooooob
approved these changes
Nov 24, 2021
| let (policy, asset_name, value) = asset_to_add; | ||
| let mut current_assets_clone = current_assets.clone(); | ||
| current_assets_clone.insert(&asset_name, &value); | ||
| let mut amount_clone = output.amount.clone(); |
There was a problem hiding this comment.
We should be able to avoid this clone but it's nothing important
|
We should squash and merge this instead of the create a merge commit option IMO. We've been having so many merging master into PR branches lately and the commit history has been getting really dirty. It might be worth doing that in general now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Abstract
This PR makes changes the
add_change_if_neededfunction from theTransactionBuilderclass to split assets among multiple outputs when generating a change output which contains a large number of assets.Motivation
While unlikely, it is possible that we run into an overflow on the size of the output when generating a change outputs which contains several assets. This can be handled by splitting these assets among multiple outputs.
Background
When we create a new instance of
TransactionBuilderwe specify a value for themax_value_size, which is a size limit for the value of each output in the transaction. If while building the change output it gets larger thanmax_value_size, the transaction building will result in an error.See the task description on Asana.
Implementation
To handle the scenario described above, instead of just inserting all the assets into the change output directly, we loop through all the asset names from the
Assetsobject which comes from the change estimatorMultiAssetand insert them one by one into a newAssetsobject, defined asrebuilt_assets, and only afterwards we insert thisrebuilt_assetsobject into theMultiAssetand then finally we add theMultiAssetinto the output.Before inserting each individual asset into the
rebuilt_assetsobject, we simulate an insert with cloned objects to verify if adding this asset to the output will cause an overflow, by using the newly definedwill_adding_asset_make_output_overflowfunction. If it doesn't cause the overflow, we just continue normally. If it does cause an overflow, we insert the currentrebuilt_assetsinto the output and generate a new one and continue inserting the remaining assets into this new output.We store all the
MultiAssetobjects from the outputs generated by thepack_nfts_for_changefunction into aVec<MultiAsset>and changepack_nfts_for_changeto return this vector instead of a singleMultiAsset. Finally, we use the same logic for adding the NFT change we had before for each of the items returned bypack_nfts_for_change