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

Integrate ZSA-related state enhancements and new getassetstate RPC call #33

Open
wants to merge 36 commits into
base: zsa-integration-demo
Choose a base branch
from

Conversation

dmidem
Copy link

@dmidem dmidem commented Dec 7, 2024

This update incorporates the latest state enhancements for ZSA, including tracking the asset total supply and finalization flag. Additionally, the getassetstate RPC call has been implemented to allow querying the state of specific assets. See PRs #26, #32, and #29 for more details.

getassetstate call be tested using the following curl command after starting the node:

curl -X POST http://127.0.0.1:18232/ \
     -H "Content-Type: application/json" \
     -d '{
           "jsonrpc": "1.0",
           "id": "curltest",
           "method": "getassetstate",
           "params": ["c9cd432edea87319b8bdf5b400d17cb0d4743f2174c15037c7fd9e5cdce94586"]
         }'

Ensure to replace the params with the correct hex-encoded asset ID (asset base) and specify the correct port and address.

You can also specify an optional boolean flag in params to include assets from the non-finalized state in the response. The flag defaults to true if omitted. For example:

...
"params": ["c9cd432edea87319b8bdf5b400d17cb0d4743f2174c15037c7fd9e5cdce94586", false]
...

arya2 and others added 30 commits November 12, 2024 01:07
…callyVerifiedBlock types, updates `IssuedAssetsChange::from_transactions()` method return type
…f BE for amount, read amount after asset base)
…ror in the function of the crate (this may not be a fully correct fix). Add a couple of FIXME comments explaining the problem.
…64 to prevent serialization errors and enable defining BurnItem in orchard, facilitating its reuse along with related functions
…tead of amount (with_asset is used to process orchard burn) - this allows avoiding the use of try_into for burn in binding_verification_key function
@dmidem dmidem requested a review from PaulLaux December 7, 2024 22:37
-A clippy::needless_lifetimes \
-A missing-docs \
-A non_local_definitions \
-A clippy::needless_return
Copy link

Choose a reason for hiding this comment

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

It's better to ignore single lines (and fix later) than disable entirely
https://stackoverflow.com/a/55402970

Comment on lines +53 to +54
// TODO: Update `Burn` to `HashMap<AssetBase, NoteValue>)` and return an error during deserialization if
// any asset base is burned twice in the same transaction
Copy link
Collaborator

@arya2 arya2 Dec 10, 2024

Choose a reason for hiding this comment

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

Actually, this could make it harder to consistently serialize in the same order, I'll add the check here.

@dmidem
Copy link
Author

dmidem commented Dec 11, 2024

@arya2 I have a couple of questions about the asset state management code.

  1. When I make a getassetstate RPC call using curl, I receive the error message "IO error: failed to fill whole buffer". This occurs when the asset base parameter in the query is invalid (e.g., an empty string). Could we return a more relevant error message in this case?

  2. Do you have any validation checks in the code to ensure that an asset has is_finalized = false in the existing state before making any new state changes to it? (@PaulLaux, or this needs to be checked in the consensus part?)

@arya2
Copy link
Collaborator

arya2 commented Dec 11, 2024

  1. When I make a getassetstate RPC call using curl, I receive the error message "IO error: failed to fill whole buffer". This occurs when the asset base parameter in the query is invalid (e.g., an empty string). Could we return a more relevant error message in this case?

Yep, thank you for catching that.

  1. Do you have any validation checks in the code to ensure that an asset has is_finalized = false before making any state changes to it? (@PaulLaux, or this needs to be checked in the consensus part?)

apply_finalization() checks that either an asset state isn't finalized or the change doesn't include any issuance, so it currently allows burning finalized assets.

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.

3 participants