add promote_transaction_api #127
Conversation
|
Thanks Scott! I will take a look later this week! |
todofixthis
left a comment
todofixthis
left a comment
There was a problem hiding this comment.
Looks really good, thanks @scottbelden !
Couple of requests. As always, just minor stuff (:
| ) | ||
|
|
||
| spam_transfer = ProposedTransaction( | ||
| address=Address(b'9' * 81), |
There was a problem hiding this comment.
💡 Address will automatically pad the value to 81 trytes. No magic number necessary (:
| 'state': True, | ||
| }) | ||
|
|
||
| result_bundle = Bundle.from_tryte_strings(self.hash2) |
There was a problem hiding this comment.
❔ Is this correct? I think Bundle.from_tryte_strings expects an array of TransactionTrytes objects.
| { | ||
| 'changeAddress': None, | ||
| 'inputs': None, | ||
| 'reference': None, |
There was a problem hiding this comment.
❔ Good thinking! Could we also include unit tests for invalid values for reference (i.e., not TrytesCompatible)?
| if value['reference'] is None: | ||
| del value['reference'] | ||
|
|
||
| return value |
There was a problem hiding this comment.
❔ Could we add a couple of unit tests for GetTransactionsToApproveRequestFilter, related to reference?
- Not TrytesCompatible.
- Included in resulting dict if not null.
scottbelden
force-pushed
the
promote_transaction
branch
from
be2f8c5 to
ab729af
Compare
|
I rebased and believe I resolved the outstanding comments. Let me know if there's anything I missed or that should still be updated. |
| min_weight_magnitude = request['minWeightMagnitude'] # type: int | ||
| transaction = request['transaction'] # type: TransactionHash | ||
|
|
||
| cc_response = CheckConsistencyCommand(self.adapter)(tails=[transaction]) |
There was a problem hiding this comment.
I wanted to use the new is_promotable helper here, but we don't have a reference to the Iota class. I don't think this is a problem as it doesn't cause much duplication, but if we have more helper functions that we want to use from these commands in the future, it might need some thought on how to achieve that.
There was a problem hiding this comment.
Hm... interesting.
What do you think about this?
class Helpers:
def __init__(self, adapter):
...Then PromoteTransactionCommand can do Helpers(self.adapter).is_promotable(...) (and minor adjustment to how Iota.helpers is initialised).
The tradeoff is that helpers won't have access to an Iota instance, but that hasn't proved to be an obstacle for command classes so far (:
There was a problem hiding this comment.
Does that mean that the current is_promotable should be changed from:
def is_promotable(self, tail):
return self.api.check_consistency(tails=[tail])['state']To:
from iota.commands.core.check_consistency import CheckConsistencyCommand
def is_promotable(self, tail):
return CheckConsistencyCommand(self.adapter)(tails=[tail])['state']I don't see a problem with that.
There was a problem hiding this comment.
Aye, I think that would... oh. Hm, we might end up with circular references that way.
Eh, I guess we could import the commands locally in the helper methods. The alternative, I think, would be to create helper classes, similar to what we're doing for commands. That feels a bit cathedral-ish to me, though; I dunno.
There was a problem hiding this comment.
Yeah, I can foresee circular dependencies. I'm okay with keeping it as is (not using the helper call) for the moment until a later date.
todofixthis
left a comment
todofixthis
left a comment
There was a problem hiding this comment.
Looks great! Thanks Scott!!
Let me know what you think we should do about Helpers. Once you're satisfied, I'll merge this for the next release (:
|
@todofixthis Would you be okay with merging this and creating an issue to later resolve the best way to use the helper functions? I'm not sure what's best at the moment but I wouldn't want to hold this up while trying to think of something. |
|
Hey Scott. Sure, that sounds fine with me! I'll go ahead and merge these changes now. |
…ansaction add promote_transaction_api
2 participants
Resolves part two of #121
This is slightly different from the JS library in that the JS library takes in the
transferas one of the arguments. I thought this was cleaner, but I can change it back to how the JS library works if desired.