feat(standards): P2ID/P2IDE notes must carry at least one asset#2986
feat(standards): P2ID/P2IDE notes must carry at least one asset#2986VAIBHAVJINDAL3012 wants to merge 1 commit into
Conversation
VAIBHAVJINDAL3012
force-pushed
the
at-least-one-asset
branch
from
c94a5cc to
af33c26
Compare
[BREAKING] Enforced at both layers: - Rust: P2idNote::create / P2ideNote::create return the new NoteError::MissingAsset on empty assets. - MASM: basic_wallet::add_assets_to_account asserts num_assets > 0 (new ERR_BASIC_WALLET_EMPTY_NOTE_ASSETS). Adds unit tests for both rejection paths. Kernel-test fixtures that constructed 0-asset P2ID notes are updated: incidental sites pass a dummy FungibleAsset; the dedicated p2id_note_0_assets scenario in TestSetup is dropped (1- and 2-asset coverage remains).
VAIBHAVJINDAL3012
force-pushed
the
at-least-one-asset
branch
from
af33c26 to
35a4f39
Compare
|
Question (without having looked at the code): do we want to do anything about a fungible asset with |
Technically we shouldn't allow it but it still doesn't have any attack vector. We can also put checks while transferring asset to account that asset value shouldn't be 0. |
This may be tricky as we can't check this for all assets in general (especially, at the protocol level) - e.g., "custom" assets would be excluded. |
Which assets have value 0? |
Fungible assets currently. In the future, we'll have "custom" asset with asset-specific structure - so, inferring whether an asset is a "zero" asset by looking at the asset itself, may not be possible in generality. But we may be able to do the check in a different way: if the root of the asset vault didn't change after the asset was added, we know that adding an asset didn't have an effect. I haven't looked into the code to know how easy/viable it is to perform this check - but it may be not too bad. |
|
|
||
| /// Return a [`TestSetup`], whose notes contain 0, 1 and 2 assets respectively. | ||
| /// Return a [`TestSetup`], whose notes contain 1 and 2 assets respectively. | ||
| fn setup_test() -> anyhow::Result<TestSetup> { |
There was a problem hiding this comment.
I think we should retain the 0 assets test case here as the tests that use this setup, like the get_assets test, behave meaningfully different from the non-zero asset use case. So, maybe we can just change the test setup to return P2ANY notes instead of P2ID notes (see create_p2any_note, add_p2any_note).
There was a problem hiding this comment.
Got it, I will make this change.
| #! Adds all assets from the active note to the native account's vault. | ||
| #! | ||
| #! Inputs: [] | ||
| #! Outputs: [] | ||
| @locals(512) | ||
| pub proc add_assets_to_account | ||
| # write assets to local memory starting at offset 0 | ||
| # we have allocated ASSET_SIZE * MAX_ASSETS_PER_NOTE number of locals so all assets should fit | ||
| # since the asset memory will be overwritten, we don't have to initialize the locals to zero | ||
| locaddr.0 exec.active_note::get_assets | ||
| # => [num_of_assets] | ||
|
|
||
| # ensure the note carries at least one asset | ||
| dup neq.0 assert.err=ERR_BASIC_WALLET_EMPTY_NOTE_ASSETS | ||
| # => [num_of_assets] | ||
|
|
There was a problem hiding this comment.
I'm not sure if this procedure, at the wallet level, should enforce "at least one asset". I think the procedure should be a no-op if no assets exist. I think we should check this at note level instead. For this, we would need to expose active_note::get_assets_info so we can somewhat cheaply get the number of assets without having to write them to memory (like active_note::get_assets would do).
Separately, at the note level, should we add the check that @bobbinth mentioned (#2986 (comment))?
There was a problem hiding this comment.
Yes, I thought over it to checks at note level, but I was making redundant checks on P2ID and P2IDE, then I thought of this approach.
we may be able to do the check in a different way: if the root of the asset vault didn't change after the asset was added, we know that adding an asset didn't have an effect. I haven't looked into the code to know how easy/viable it is to perform this check - but it may be not too bad.
Will try for this check.
There was a problem hiding this comment.
Yes, I thought over it to checks at note level, but I was making redundant checks on P2ID and P2IDE, then I thought of this approach.
I think the original motivation was to have this check for P2ID* notes, but with this change, we'd also affect other notes that use this API, which may or may not want to implement this check. I think the add_assets_to_account API is more useful if it doesn't require a non-zero number of assets. If users would want that behavior with the latest changes, they'd have to wrap it in an if-else branch.
So, I'd move these checks to the P2ID* notes. I don't think the duplication is an issue in this case.
There was a problem hiding this comment.
Okay, I will implement this in P2ID and P2IDE note.
partylikeits1983
left a comment
partylikeits1983
left a comment
There was a problem hiding this comment.
Maybe I am missing something, but why does the MASM procedure add_assets_to_account need to check that that the Asset is not zero? Why would it need to revert if the amount is 0?
I think its valuable to add the checks to the note builder rust logic that an Asset amount cannot be zero when creating a note, but I am not sure if this logic should be in the masm logic
4 participants
This PR add the assertion check for the case that there should be atleast one asset in the note when
add_assets_to_accountfunction is called.[BREAKING] Enforced at both layers:
closes #2978