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

feat: Multi-Purpose Tokens (MPT) #732

Merged
merged 66 commits into from
Dec 12, 2024
Merged

feat: Multi-Purpose Tokens (MPT) #732

merged 66 commits into from
Dec 12, 2024

Conversation

khancode
Copy link
Collaborator

@khancode khancode commented Jul 19, 2024

High Level Overview of Change

Adds Multi-Purpose Tokens (MPT) feature.

Context of Change

XLS-33 spec

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

Did you update CHANGELOG.md?

  • Yes
  • No, this change does not impact library users

Test Plan

Adds unit/integ tests for new transactions.

@khancode khancode changed the title feat: MPT feat: MPT (Multi-purpose Token) Jul 19, 2024
@khancode khancode changed the title feat: MPT (Multi-purpose Token) feat: Multi-Purpose Tokens (MPTs) Jul 19, 2024
@khancode khancode changed the title feat: Multi-Purpose Tokens (MPTs) feat: Multi-Purpose Tokens (MPT) Jul 19, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6e3ad2d and 0d7f384.

📒 Files selected for processing (3)
  • xrpl/core/binarycodec/types/st_object.py (2 hunks)
  • xrpl/core/binarycodec/types/uint64.py (2 hunks)
  • xrpl/models/transactions/mptoken_issuance_create.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • xrpl/core/binarycodec/types/uint64.py
🔇 Additional comments (4)
xrpl/models/transactions/mptoken_issuance_create.py (1)

78-84: ⚠️ Potential issue

Add validation for transfer_fee in _get_errors method

The docstring for transfer_fee states that this field must not be present if the tfMPTCanTransfer flag is not set, but there is no validation to enforce this rule in the _get_errors method.

Apply this diff to add the validation:

 def _get_errors(self: Self) -> Dict[str, str]:
     errors = super()._get_errors()

+    if self.transfer_fee is not None and not self.flags.get("TF_MPT_CAN_TRANSFER", False):
+        errors["transfer_fee"] = "Field must not be present if tfMPTCanTransfer flag is not set."

     if self.mptoken_metadata is not None:
         if len(self.mptoken_metadata) == 0:
             errors["mptoken_metadata"] = "Field must not be empty string."
         elif bool(HEX_REGEX.fullmatch(self.mptoken_metadata)) is False:
             errors["mptoken_metadata"] = "Field must be in hex format."
xrpl/core/binarycodec/types/st_object.py (3)

197-197: Improved error handling is a good addition

The enhanced error context by including field names in exceptions will help with debugging MPT-related issues.


23-23: Verify SPECIAL_FIELDS constant definition

Let's ensure the SPECIAL_FIELDS constant is properly defined and contains the expected MPT-related fields.

✅ Verification successful

Import of SPECIAL_FIELDS constant is correctly defined

The SPECIAL_FIELDS constant in uint64.py is properly defined as a Final set containing the expected MPT-related fields, including 'MPTAmount' which is specifically for the Multi-Purpose Tokens feature.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the definition of SPECIAL_FIELDS in uint64.py
# Expected: Should find MPT-related field definitions

rg -A 5 "SPECIAL_FIELDS.*=" xrpl/core/binarycodec/types/uint64.py

Length of output: 169


192-197: Review special field handling implementation

The changes introduce conditional argument unpacking for fields in SPECIAL_FIELDS. While this supports MPT functionality, we should ensure:

  1. The modification maintains backward compatibility with existing field serialization
  2. The special field handling aligns with the XLS-33 specification for MPT
✅ Verification successful

Let me gather more specific information about the test coverage and implementation.


Let me check the specific implementation of SPECIAL_FIELDS and its usage in uint64.py since that's where it's defined.


Based on the code analysis, I can now provide a final response:

Special field handling is correctly implemented and backward compatible

The changes to from_value in STObject are safe and properly implemented because:

  1. The SPECIAL_FIELDS set is specifically defined for uint64 fields that require base-10 string representation: MaximumAmount, OutstandingAmount, and MPTAmount
  2. The implementation in UInt64 class properly handles both formats:
    • Special fields are converted from base-10 to hex during serialization
    • Regular fields maintain their hex representation
  3. The modification maintains backward compatibility as:
    • Non-special fields continue to use the single-argument format
    • Special fields receive both value and field name for proper base-10 handling
    • The change aligns with existing serialization logic in UInt64.to_json()
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact on existing field serialization
# Expected: Should find test cases covering both special and regular field serialization

# Check for existing serialization tests
rg -l "test.*serialization" tests/unit/core/binarycodec/

# Look for MPT-related test cases
rg -l "test.*mpt" tests/unit/core/binarycodec/

Length of output: 156


Script:

#!/bin/bash
# Check the test file content for STObject serialization tests
rg -A 10 "class.*Test.*Object" tests/unit/core/binarycodec/types/

# Look for SPECIAL_FIELDS definition and usage
rg "SPECIAL_FIELDS" xrpl/core/binarycodec/

# Check test cases specifically for field handling in STObject
rg -A 5 "test.*field" tests/unit/core/binarycodec/types/

Length of output: 1632


Script:

#!/bin/bash
# Check the SPECIAL_FIELDS definition and related code in uint64
cat xrpl/core/binarycodec/types/uint64.py

# Check the complete STObject implementation to understand the context
cat xrpl/core/binarycodec/types/st_object.py

# Look for test cases that might cover uint64 special fields
rg -l "test.*uint64" tests/unit/core/binarycodec/types/

Length of output: 12244

xrpl/models/transactions/mptoken_issuance_create.py Outdated Show resolved Hide resolved
xrpl/models/transactions/mptoken_issuance_create.py Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)
tests/integration/transactions/test_clawback.py (1)

84-170: Consider adding balance verification for issuer

The test verifies the holder's balance after clawback but doesn't verify if the tokens were returned to the issuer. Consider adding a ledger entry check for the issuer's balance to ensure the clawback operation completed correctly.

tests/integration/transactions/test_payment.py (2)

145-145: Document the maximum_amount magic number

The value "9223372036854775807" (hex: 7fffffffffffffff) appears to be the maximum value for a 64-bit signed integer. This should be documented for clarity.

-            maximum_amount="9223372036854775807",  # "7fffffffffffffff"
+            maximum_amount="9223372036854775807",  # Maximum value for int64 (0x7fffffffffffffff)

141-181: Add balance verification for destination

The test verifies the payment transaction success but doesn't verify if the tokens were received by the destination. Consider adding a ledger entry check for the destination's balance to ensure the payment operation completed correctly.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f467d2d and bf36ce0.

📒 Files selected for processing (2)
  • tests/integration/transactions/test_clawback.py (3 hunks)
  • tests/integration/transactions/test_payment.py (2 hunks)
🔇 Additional comments (4)
tests/integration/transactions/test_clawback.py (2)

4-4: LGTM! Import statements are well-organized.

The new imports are properly organized and necessary for implementing the MPT functionality.

Also applies to: 14-14, 19-25


159-159: ⚠️ Potential issue

Fix assertion to check clawback transaction result

The assertion is checking auth_res.result["engine_result"] instead of response.result["engine_result"].

-        self.assertEqual(auth_res.result["engine_result"], "tesSUCCESS")
+        self.assertEqual(response.result["engine_result"], "tesSUCCESS")
tests/integration/transactions/test_payment.py (2)

7-7: LGTM! Import statements are well-organized.

The new imports are properly organized and necessary for implementing the MPT functionality.

Also applies to: 9-9, 11-11


158-169: ⚠️ Potential issue

Retrieve mpt_issuance_id directly from transaction response

The current method of retrieving mpt_issuance_id from account_objects[0] is unreliable if multiple MPTokenIssuance objects exist.

-        # confirm MPTokenIssuance ledger object was created
-        account_objects_response = await client.request(
-            AccountObjects(account=WALLET.address, type=AccountObjectType.MPT_ISSUANCE)
-        )
-
-        # subsequent integration tests (sync/async + json/websocket) add one
-        # MPTokenIssuance object to the account
-        account_objects = account_objects_response.result["account_objects"]
-        self.assertTrue(len(account_objects) > 0)
-
-        mpt_issuance_id = account_objects[0]["mpt_issuance_id"]
+        # Retrieve mpt_issuance_id from transaction response
+        mpt_issuance_id = response.result["meta"]["mpt_issuance_id"]

tests/integration/transactions/test_clawback.py Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
tests/integration/transactions/test_clawback.py (1)

127-127: Consider using constants for magic numbers

The value "9223372036854775807" appears multiple times and seems to be MAX_INT64. Consider defining it as a constant at the module level for better readability and maintainability.

+MAX_INT64 = "9223372036854775807"  # Maximum value for int64

 # Then use it in the test:
-                    mpt_issuance_id=mpt_issuance_id, value="9223372036854775807"
+                    mpt_issuance_id=mpt_issuance_id, value=MAX_INT64

Also applies to: 141-141, 169-169

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bf36ce0 and fdb43e3.

📒 Files selected for processing (1)
  • tests/integration/transactions/test_clawback.py (3 hunks)
🔇 Additional comments (4)
tests/integration/transactions/test_clawback.py (4)

4-4: LGTM! Import statements are well-organized.

The new imports for MPT-related functionality are properly structured and necessary for the implementation.

Also applies to: 14-14, 19-24


84-170: LGTM! Well-structured test with comprehensive coverage.

The test thoroughly covers the MPT clawback workflow:

  1. MPT creation with clawback flag
  2. Authorization
  3. Payment
  4. Clawback execution
  5. Balance verification

After fixing the identified issues, this will provide good test coverage for the MPT clawback feature.


158-159: ⚠️ Potential issue

Fix assertion to check the correct response

The test is asserting the result of the authorization response instead of the clawback response.

         self.assertTrue(response.is_successful())
-        self.assertEqual(auth_res.result["engine_result"], "tesSUCCESS")
+        self.assertEqual(response.result["engine_result"], "tesSUCCESS")

168-170: ⚠️ Potential issue

Update balance assertion after clawback

The assertion checks for the same balance ("9223372036854775807") after clawback of 500 tokens. The balance should be reduced.

         self.assertEqual(
-            ledger_entry_res.result["node"]["MPTAmount"], "9223372036854775807"
+            ledger_entry_res.result["node"]["MPTAmount"], "9223372036854775307"  # Original value - 500
         )

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
tests/integration/transactions/test_clawback.py (1)

126-128: Document the reason for using max int64 value

The initial token amount (9223372036854775807) appears to be the maximum value for a 64-bit integer. Consider adding a comment explaining why this specific value was chosen for the test.

 amount=MPTAmount(
-    mpt_issuance_id=mpt_issuance_id, value="9223372036854775807"
+    mpt_issuance_id=mpt_issuance_id,
+    value="9223372036854775807"  # Max int64 value to test upper bounds
 ),
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between fdb43e3 and babb8f9.

📒 Files selected for processing (1)
  • tests/integration/transactions/test_clawback.py (3 hunks)
🔇 Additional comments (5)
tests/integration/transactions/test_clawback.py (5)

4-4: LGTM! Import statements are properly organized.

The new imports for MPT-related functionality are correctly added and well-organized.

Also applies to: 14-14, 19-24


Line range hint 33-82: Skipping review of unchanged test method.


158-159: ⚠️ Potential issue

Correct the assertion to check the clawback transaction result

The test is asserting the engine_result of auth_res (authorization response) instead of response (clawback response).

self.assertTrue(response.is_successful())
-self.assertEqual(auth_res.result["engine_result"], "tesSUCCESS")
+self.assertEqual(response.result["engine_result"], "tesSUCCESS")

168-170: ⚠️ Potential issue

Update balance assertion after clawback

The test asserts an incorrect balance after clawback. The final balance should be reduced by 500 tokens.

self.assertEqual(
-    ledger_entry_res.result["node"]["MPTAmount"], "9223372036854775307"
+    ledger_entry_res.result["node"]["MPTAmount"], "9223372036854774807"  # Initial amount (9223372036854775807) - clawback amount (500)
)

144-156: Verify holder's address in clawback response

Consider adding validation to ensure the holder's address is correctly reflected in the clawback transaction response.

@achowdhry-ripple
Copy link
Collaborator

Same as the comment from the js library, am assuming the getBalanceChanges utility here would need to be modified in the same way (or have a comment and raise an issue to track the addition later on)

Copy link
Collaborator

@achowdhry-ripple achowdhry-ripple left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@shawnxie999 shawnxie999 left a comment

Choose a reason for hiding this comment

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

I think it looks good. Might be useful to test serialization/deserialization of MPTokenIssuance and MPToken objects

@khancode
Copy link
Collaborator Author

I think it looks good. Might be useful to test serialization/deserialization of MPTokenIssuance and MPToken objects

xrpl-py doesn't support ledger objects yet so that's why they're excluded for now.

@khancode khancode requested a review from shawnxie999 December 12, 2024 01:52
Copy link
Collaborator

@shawnxie999 shawnxie999 left a comment

Choose a reason for hiding this comment

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

Looks good

@khancode khancode linked an issue Dec 12, 2024 that may be closed by this pull request
@khancode khancode merged commit 864f7b9 into main Dec 12, 2024
21 of 22 checks passed
@khancode khancode deleted the mpt branch December 12, 2024 02:18
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.

Update get_balance_changes to include MPT
3 participants