Trade announcement #30
Conversation
…catch rpc_store calls. Added a callback function in peer which is called when we catch a new key value pair on the network.
…p instances of NewServer and check that we are catching set commands in the DHT, triggering our callback function \n Testing coverage for the server module is at 100%
updated commission object and peer initialization
There was a problem hiding this comment.
Mostly good. I really like the readability with the self-documenting names and the literate programming effect that results.
A couple of things to look at though and some optionals to consider.
Nothing severe enough to hold this up, so I'm going to merge it to keep things moving for our Monday deadline. Please address the necessary issues in a follow-up PR.
| return self.commissions[key] | ||
| raise KeyError(f"No commission found for key: {key}") | ||
|
|
||
| def get_owned_artwork(self, key: bytes): |
There was a problem hiding this comment.
Optional/Opportunistic Improvement
Similar to a discussion from Yen's code review, Python has a different approach to/view of encapsulation. It doesn't have true private members, though you can emulate by preceding class & instance variables with either a single or double underscore.
Full discussion of this topic is too much for here. But, all of that context to was set up a comment that this, and the above, getter could be removed. Python convention considers it okay to access a member like commissions and owned_artworks directly by external callers.
I would keep is_owned_artwork below though. I think it adds to the readability.
| self.logger.info("Trade announced") | ||
| else: | ||
| self.logger.error("Trade failed to announce") |
There was a problem hiding this comment.
Please address, if appropriate for debug-ability.
These two log messages are the same as below in announce_trade. Shouldn't they differ in some manner?
| trade_key, artwork_to_trade.key, self.keys["public"] | ||
| ) | ||
| response_key = utils.generate_random_sha1_hash() | ||
| self.inventory.add_pending_trade( |
There was a problem hiding this comment.
Please address.
The pending trade should only be added on success. So this should be moved to the if set_success branch below.
| async def handle_accept_trade(self, response: OfferResponse): | ||
| """Handle an accepted trade""" | ||
|
|
||
| self.logger.info(response) | ||
|
|
||
| async def handle_reject_trade(self, response: OfferResponse): | ||
| """Handle a rejected trade""" | ||
|
|
||
| self.logger.info(response) |
| if trade_key in self.inventory.pending_trades: | ||
| self.inventory.remove_pending_trade(trade_key) | ||
| if response.trade_id in self.inventory.pending_trades: |
There was a problem hiding this comment.
Please remind me... What's the scenario where trade_key would be different from response.trade_id? Why would they both be in pending_trades?
|
|
||
| def generate_random_sha1_hash(): | ||
| """ | ||
| Generates a random SHA-1 hash as a file descriptor for the trade response. |
There was a problem hiding this comment.
Optional/Opportunistic Improvement
Given this function is now in a utils module, the doc should be more general.
Generates a random SHA-1 hash as a file descriptor for the trade response.
Something like:
Generates a random SHA-1 hash that can be used as an ID for cases where there's no obvious attribute on which to base the ID.
| def get_artwork_ledger_key(self): | ||
| """ | ||
| Returns the artwork to be traded. | ||
| """ | ||
| return self.artwork_ledger_key | ||
|
|
||
| def get_originator_public_key(self): | ||
| """ | ||
| Returns the public key of the originator of the trade announcement. | ||
| """ | ||
| return self.originator_public_key |
There was a problem hiding this comment.
Similar comment as above about Python encapsulation, getters.
| def get_trade_id(self): | ||
| """ | ||
| Returns the trade id to respond to. | ||
| """ | ||
| return self.trade_id | ||
|
|
||
| def get_originator_public_key(self): | ||
| """ | ||
| Returns the public key of the originator of the trade response. | ||
| """ | ||
| return self.originator_public_key | ||
|
|
||
| def get_offer_ledger_id(self): | ||
| """ | ||
| Returns the offer ledger id to respond to. | ||
| """ | ||
| return self.offer_ledger_id |
There was a problem hiding this comment.
Similar comment as above about Python encapsulation, getters.
| """ | ||
|
|
||
|
|
||
| class OfferResponse: |
There was a problem hiding this comment.
Optional/Opportunistic Improvement
This could also be a @dataclass (see above).
| """ | ||
|
|
||
|
|
||
| class OfferAnnouncement: |
There was a problem hiding this comment.
Optional/Opportunistic Improvement
Consider making this a @dataclass. That generates a lot of boilerplate code associated with classes like this.
Created initial functionality for peers to announce and respond to trades