Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

Added mint event type to NotificationDB #433

Merged
merged 3 commits into from
May 29, 2018

Conversation

metachris
Copy link
Contributor

@metachris metachris commented May 24, 2018

What current issue(s) does this address, or what feature is it adding?

Added mint events to the NotificationDB.

How did you solve this problem?

Updated NotificationDB.py and SmartContractEvent.py

How did you make sure your solution works?

Added a test, but not live tested.

Couldn't get any current mint events on my bootstrapped chains. Is there an easy way to test it, besides on a private net and deploying a NEP-5 SC? (Reminds me, we should a NEP-5 SC to the privatenet default)

Are there any special changes in the code that we should be aware of?

No

Please check the following, if applicable:

  • Did you add any tests?
  • Did you run make lint?
  • Did you run make test?
  • Are you making a PR to a feature branch or development rather than master?
  • Did you add an entry to CHANGELOG.rst? (if not, please do)

@@ -231,11 +229,18 @@ def __init__(self, event_type, event_payload, contract_hash, block_number, tx_ha
self.amount = int(BigInteger.FromBytes(event_payload[3])) if isinstance(event_payload[3], bytes) else int(event_payload[3])
self.is_standard_notify = True

elif plen == 3 and self.notify_type == NotifyType.REFUND:
elif self.notify_type == NotifyType.REFUND and plen >= 3: # Might have more arguments
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saw some contracts that have 4 arguments, using this signature:

OnRefund = RegisterAction('refund', 'addr_to', 'amount', 'asset')

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting, I wonder what the 4th argument is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whether it's gas or neo?

@localhuman localhuman merged commit 096b1a6 into development May 29, 2018
elif self.notify_type == NotifyType.MINT and plen == 3:
self.addr_to = UInt160(data=self.event_payload[1]) if len(self.event_payload[1]) == 20 else empty
self.amount = int(BigInteger.FromBytes(event_payload[2])) if isinstance(event_payload[2], bytes) else int(event_payload[2])
self.addr_from = self.contract_hash
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct approach? I know that there isn't a defined standard around minting yet, but many smart contracts use a null from address for the minting operations. I realize this is just for the mint SC event notification, but I do wonder if this would be expected for someone writing an event handler. Probably not a big deal, but I thought I'd raise it as a consideration anyway...

@ThomasLobker
Copy link
Contributor

In my database I'm considering the all zeroes public key as the "minting source", since most contracts seem to have used that approach. I would encourage this approach to be the standard, to keep behavior uniform.

@metachris metachris mentioned this pull request Jun 5, 2018
@metachris metachris deleted the notificationdb-mint-event branch June 6, 2018 18:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants