Skip to content

Allow optional arguments in controllers#1366

Merged
michwill merged 14 commits intonucypher:masterfrom
michwill:control_optional
Oct 1, 2019
Merged

Allow optional arguments in controllers#1366
michwill merged 14 commits intonucypher:masterfrom
michwill:control_optional

Conversation

@michwill
Copy link
Contributor

@michwill michwill commented Sep 28, 2019

  • Optional arguments in RPC controllers
  • Pass value/rate/first_period_reward arguments optionally to make blockchain grant working over RPC (how did that work at all?)
  • Tests RPC with blockchain characters

@codecov
Copy link

codecov bot commented Sep 28, 2019

Codecov Report

Merging #1366 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1366      +/-   ##
==========================================
+ Coverage   83.02%   83.03%   +<.01%     
==========================================
  Files          72       72              
  Lines       10457    10333     -124     
==========================================
- Hits         8682     8580     -102     
+ Misses       1775     1753      -22
Impacted Files Coverage Δ
nucypher/characters/control/interfaces.py 96.29% <100%> (+0.03%) ⬆️
nucypher/characters/control/serializers.py 86.29% <100%> (+0.33%) ⬆️
nucypher/characters/control/specifications.py 92.72% <100%> (+0.27%) ⬆️
nucypher/cli/deploy.py 74.66% <0%> (-2.99%) ⬇️
nucypher/blockchain/eth/registry.py 76.22% <0%> (-1.52%) ⬇️
nucypher/cli/actions.py 73.44% <0%> (-1.05%) ⬇️
nucypher/network/nodes.py 81.37% <0%> (-0.46%) ⬇️
nucypher/network/server.py 85.13% <0%> (-0.46%) ⬇️
nucypher/blockchain/eth/token.py 89.32% <0%> (-0.36%) ⬇️
nucypher/blockchain/eth/actors.py 85.78% <0%> (-0.3%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1cc8b7...d0c889a. Read the comment docs.

@michwill michwill changed the title [WIP] Allow optional arguments in controllers Allow optional arguments in controllers Sep 29, 2019
@@ -0,0 +1,107 @@
import pytest
Copy link
Member

Choose a reason for hiding this comment

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

Are these new intentionally test modules (added the letter b)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow pytest isn't happy when they have the same filename, albeit in different directories. Thus, _b / _f

Copy link
Member

Choose a reason for hiding this comment

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

Can we combine them into a single test - or does this need to be a stand alone module?



@pytest.fixture(scope="module")
def capsule_side_channel_blockchain(enacted_blockchain_policy):
Copy link
Member

Choose a reason for hiding this comment

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

Interesting - What Incompatibility did you encounter with the Federated Side Channel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, they have different labels, so it pretty much appears to be the wrong capsule (and yielding a decryption error). It's not incompatibility per se, I think fixtures can be worked on. I just didn't focus on that in this PR - wanted them minimally working.

raise cls.SpecificationError(f"{cls.__class__.__name__} has no such control interface: '{interface_name}'")

return input_specification, output_specification
return spec.get('in', tuple()),\
Copy link
Member

Choose a reason for hiding this comment

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

This bit reads a little awkward on the return line, especially.

This comment was marked as off-topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Because all in should be present in the spec?

__create_policy = (('bob_encrypting_key', 'bob_verifying_key', 'm', 'n', 'label', 'expiration'), # In
('label', 'policy_encrypting_key')) # Out
__create_policy = {'in': ('bob_encrypting_key', 'bob_verifying_key', 'm', 'n', 'label', 'expiration'),
'optional': ('value', 'first_period_reward', 'rate'),
Copy link
Member

Choose a reason for hiding this comment

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

👍 - Excellent improvement


def reset(self):
self.messages = []
self()
Copy link
Member

Choose a reason for hiding this comment

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

🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah?
I think, I just copied it :-) Could be done better, yes

Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

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

Looking Good - Just a few few questions regarding test modules and resolving specifications.

Copy link
Contributor

@vepkenez vepkenez left a comment

Choose a reason for hiding this comment

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

💯

raise cls.SpecificationError(f"{cls.__class__.__name__} has no such control interface: '{interface_name}'")

return input_specification, output_specification
return spec.get('in', tuple()),\

This comment was marked as off-topic.

@KPrasch
Copy link
Member

KPrasch commented Oct 1, 2019

Well - I personally do prefer to have named objects before returning them for debugging ease and readability. :-)

@michwill
Copy link
Contributor Author

michwill commented Oct 1, 2019

@KPrasch ok, will return dict perhaps. I understand how that is better

@michwill
Copy link
Contributor Author

michwill commented Oct 1, 2019

Ok, as per conversation, will do namedtuple!

@michwill michwill merged commit 91f0f0a into nucypher:master Oct 1, 2019
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