Skip to content
This repository has been archived by the owner on Jan 13, 2023. It is now read-only.

Update the core commands to match the latest IRI 1.8.1 release #236

Merged
merged 3 commits into from
Sep 30, 2019
Merged

Update the core commands to match the latest IRI 1.8.1 release #236

merged 3 commits into from
Sep 30, 2019

Conversation

pdecol
Copy link
Contributor

@pdecol pdecol commented Sep 21, 2019

resolves #235

@pdecol
Copy link
Contributor Author

pdecol commented Sep 21, 2019 •
edited
Loading

I usually like it to clean up code that I touch. Since making the code PEP8 conform is still work in progress I changed the indentation level to 4 spaces in existing test files where I added more tests. That is the reason why this pull request has quite a lot of line changes. If its preferred to have these changes in a separate ticket/pull request let me know and I can change it back to 2 spaces.

pdecol
pdecol commented Sep 21, 2019
View reviewed changes
iota/commands/core/get_balances.py

class GetBalancesResponseFilter(ResponseFilter):
def __init__(self):
super(GetBalancesResponseFilter, self).__init__({
'balances': f.Array | f.FilterRepeater(f.Int),

'milestone':
f.ByteString(encoding='ascii') | Trytes(Address),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is a potential backwards incompatible change. milestone was removed in IRI version 1.4.1.6 (https://github.com/iotaledger/iri/blob/v1.4.1.6/src/main/java/com/iota/iri/service/dto/GetBalancesResponse.java) which was released on the 6th of January 2018. Since this was more than one and a half years ago I think it is justified to remove it here too.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 Sounds good to me.

@todofixthis
Copy link
Contributor

Hi @pdecol thanks for working on this! Really appreciate the attention to detail and being proactive about keeping the codebase consistent!

Will have a look at this today 👍🏻

lzpap
lzpap reviewed Sep 23, 2019
View reviewed changes
Copy link
Member

@lzpap lzpap left a comment

Choose a reason for hiding this comment

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

amazing, thank you for your contribution :)

iota/commands/core/get_missing_transactions.py Show resolved Hide resolved
test/commands/core/get_node_api_configuration_test.py Outdated Show resolved Hide resolved
todofixthis
todofixthis suggested changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@todofixthis todofixthis left a comment

Choose a reason for hiding this comment

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

Minor changes requested, but generally looks really good, thanks @pdecol!

@lzpap we need to come to a decision about whether/how to validate responses coming from the IRI node (survey app developer community and see if there's a clear preference one way or the other, maybe?).

iota/commands/core/get_balances.py

class GetBalancesResponseFilter(ResponseFilter):
def __init__(self):
super(GetBalancesResponseFilter, self).__init__({
'balances': f.Array | f.FilterRepeater(f.Int),

'milestone':
f.ByteString(encoding='ascii') | Trytes(Address),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻 Sounds good to me.

iota/commands/core/check_consistency.py Outdated Show resolved Hide resolved
iota/commands/core/get_missing_transactions.py Show resolved Hide resolved
iota/commands/core/get_node_api_configuration.py Outdated Show resolved Hide resolved
iota/commands/core/were_addresses_spent_from.py Outdated Show resolved Hide resolved
test/commands/core/get_node_api_configuration_test.py Outdated Show resolved Hide resolved
test/commands/core/get_balances_test.py Outdated Show resolved Hide resolved
test/commands/core/get_balances_test.py Outdated Show resolved Hide resolved
test/commands/core/get_balances_test.py Outdated Show resolved Hide resolved
test/commands/core/were_addresses_spent_from_test.py Outdated Show resolved Hide resolved
@lzpap
Copy link
Member

lzpap commented Sep 23, 2019

Minor changes requested, but generally looks really good, thanks @pdecol!

@lzpap we need to come to a decision about whether/how to validate responses coming from the IRI node (survey app developer community and see if there's a clear preference one way or the other, maybe?).

Hey @todofixthis I will start with coordinating with the other libs to see if they validate requests/responses. I am generally for filters in both directions. Then you could handle exceptions and also it might be easier to reveal bugs with future IRI API changes.

@lzpap
Copy link
Member

lzpap commented Sep 24, 2019

Minor changes requested, but generally looks really good, thanks @pdecol!
@lzpap we need to come to a decision about whether/how to validate responses coming from the IRI node (survey app developer community and see if there's a clear preference one way or the other, maybe?).

Hey @todofixthis I will start with coordinating with the other libs to see if they validate requests/responses. I am generally for filters in both directions. Then you could handle exceptions and also it might be easier to reveal bugs with future IRI API changes.

@todofixthis I talked with the devs of the other libraries, and the conclusion is that responses are not filtered/validated/checked in general, except bundle and transactions hashes. I think it would be nice to make PyOTA consistent with this behavior.

@pdecol
Copy link
Contributor Author

pdecol commented Sep 28, 2019 •
edited
Loading

Thanks for the review! I implemented the requested changes and reverted the changes to the response filters for the fields that we decided not to validate.

todofixthis
todofixthis reviewed Sep 29, 2019
View reviewed changes
Copy link
Contributor

@todofixthis todofixthis left a comment

Choose a reason for hiding this comment

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

Two more changes requested; apologies for not bringing them up during the first review. Once these last two items are addressed, this is good to go!

@pdecol
Copy link
Contributor Author

pdecol commented Sep 29, 2019 •
edited
Loading

No worries. Thanks for spotting this mistake. I removed f.Required and f.Unicode and I added f.ByteString so that it is consistent with the response filters in findTransaction, getTrytes, getTips and getMissingTransactions.

One more thing that I would like to ask you. Can you change this URL here in GitHub? (https://iota.readme.io/ -> https://docs.iota.org/) I think this should be the very last URL that needs to be adjusted.

image

@todofixthis
Copy link
Contributor

LGTM Thanks @pdecol !!

@lzpap
Copy link
Member

lzpap commented Sep 30, 2019

Thanks @pdecol !

@lzpap lzpap merged commit 7617d2b into iotaledger:develop Sep 30, 2019
@pdecol pdecol deleted the feature/235-update-core-api branch October 10, 2019 14:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@todofixthis todofixthis todofixthis approved these changes

@lzpap lzpap lzpap approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pdecol @todofixthis @lzpap