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

Fix for sending message with ESDTTransfer for transaction decoder #186

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

popenta
Copy link
Collaborator

@popenta popenta commented Jan 20, 2025

No description provided.

@popenta popenta self-assigned this Jan 20, 2025
Copy link

github-actions bot commented Jan 20, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  multiversx_sdk/network_providers
  transaction_decoder.py 122-123
Project Total  

This report was generated by python-coverage-comment-action

@@ -15,6 +15,10 @@ def __init__(self) -> None:
self.function_name: Optional[str] = None
self.function_args: Optional[list[str]] = None
self.transfers: Optional[list[TokenTransfer]] = None
self.message: Optional[list[str]] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

If list, then plural. Also, can be transfer_messages, maybe? I think bytes, instead of str? Also, can be non-optional. Empty is not applicable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renamed and changed typed to bytes.

@@ -106,9 +114,15 @@ def get_esdt_transaction_metadata(self, metadata: TransactionMetadata) -> Option
result.value = value
result.transfers = []

receiver = Address.new_from_bech32(result.receiver)
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the same case for NFT transfers or multi transfers? In that case, I think receiver.is_smart_contract() would not be appropriate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did the same for nft and multi transfer.

@@ -89,7 +89,7 @@ def get_normal_transaction_metadata(self, transaction: TransactionOnNetwork) ->
if len(args) == 0 and not transaction.receiver.is_smart_contract():
metadata.function_name = "transfer"
metadata.function_args = []
metadata.message = data_components
metadata.transfer_messages = [data.encode() for data in data_components]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since transfer_messages holds an array of bytes, and data_components contains hex-encoded components, apply a decoding from hex, to get the raw byes? Sorry if I'm mistaken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -159,8 +159,11 @@ def get_nft_transfer_metadata(self, metadata: TransactionMetadata) -> Optional[T
result.transfers = []

if len(args) > 4:
result.function_name = self.hex_to_string(args[4])
result.function_args = args[5:]
if receiver.is_smart_contract():
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, receiver is the actual receiver of the tokens, not tx.receiver, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, the actual receiver

danielailie
danielailie previously approved these changes Jan 20, 2025
@popenta popenta merged commit 46420dc into feat/next Jan 21, 2025
6 checks passed
@popenta popenta deleted the transaction-decoder branch January 21, 2025 08:04
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