-
Notifications
You must be signed in to change notification settings - Fork 106
Remove asset_witnesses field from TransactionInputs
#2298
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
cfea856
refactor: remove `asset_witnesses` field from `TransactionInputs`
bobbinth d75af0e
chore: update changelog
bobbinth 41d6978
Merge branch 'next' into bobbin-asset-witness
bobbinth 168f865
feat: add TransactionInputs::read_vault_asset()
bobbinth 7139863
feat: skip fetching asset witnesses if they are in the tx inputs
bobbinth b6ee9de
chore: update changelog
bobbinth d26357d
Merge branch 'next' into bobbin-asset-witness
bobbinth 2ef192c
Merge branch 'next' into bobbin-asset-witness
bobbinth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
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.
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 the approach from #2099 more or less strongly requires that we extend both
self.advice_inputsandself.tx_args.advice_inputs. I'm actually not sure how necessary this is (I find this pretty hard to think about now), but it seems it would be more consistent. So I would add the function added in #2274 for this:And then here do:
This reuses the existing asset witness -> advice input conversion and we don't duplicate 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'm not sure this is needed because
TransactionInputs::set_advice_inputs()should already take care of this. But I agree the whole structure is confusing. I hope that with the structure I described in #1286 (comment), we'd be able to clarify the structure somewhat.Uh oh!
There was an error while loading. Please reload this page.
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.
Technically yes, but it's not obvious that
set_advice_inputsis called later (only after tx execution) and so this leaves this "invariant" temporarily broken. Not sure if this is a real problem, and I think it's fine to address this in #1286 or #2293.Nit: I would prefer using
TransactionAdviceInputshere so we use the lower-level leaf -> advice inputs conversion implementation (TransactionAdviceInputs::add_asset_witness) rather than move it outside that type.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've actually removed
TransactionAdviceInputs::add_asset_witness()in this PR since it was no longer needed. I think we can bring it back if needed depending on how we address #1286 and #2293.