Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1285 cust auth dev #1

Open
wants to merge 122 commits into
base: 1285-cust-auth
Choose a base branch
from
Open

1285 cust auth dev #1

wants to merge 122 commits into from

Conversation

RomaMokych
Copy link

I’ve implemented eq, not eq, any, none, contains* restrictions.
Also I’ve added some base logic for authorities validation.

That logic is not integrated into transaction validating. I’m going to implement it in the beginning of the next week.

Also I’m going to improve various type support as arguments for restrictions. Now only several types are supported.

RomaMokych and others added 30 commits November 27, 2018 15:27
@RomaMokych
Copy link
Author

RomaMokych commented Feb 14, 2019

@sschiessl-bcp

Can you please give us a general update in terms of what is completed, what needs review / what is outstanding and what your estimate for the remaining tasks is?

Sure. It is left to:

  1. Finish with attribute assert - will push it today, it is under internal review now.
  2. Fix authority validation logic in custom authorities validations - will push it today, it is under internal review now.
  3. ordered_non_unqinue replacement with oredered_unique noticed by Abit - we are in the middle of discussion now. I will experiment with Abit proposal, but I'm still not sure if it will work or not. I think we will finish with today/tomorrow.
  4. Several minor fixes, some additional tests. Going to finish them today/tomorrow.

Will require review:

  1. attribute assert - will push it today, it is under internal review now.
  2. authority validation logic in custom authorities validations - will push it today, it is under internal review now.
  3. Fee calculation logic - as I understood current solution is fine. But we still need to decide how units be calculated for each type.

Everything else is done.

@xeroc
Copy link

xeroc commented Feb 14, 2019

3\. ordered_non_unqinue replacement with oredered_unique noticed by Abit - we are in the middle of discussion now. I will experiment with Abit proposal, but I'm still not sure if it will work or not. I think we will finish with today/tomorrow.

I have recently done something similar for another project:
peerplays-network/peerplays@9946eab
Works perfectly by extending the index into a composite_key with the object_id

3\. 

Everything else is done.

Excellent news. I'll look for someone to do the review.
please ping us when you have pushed the rest of the code. Thanks

@RomaMokych
Copy link
Author

RomaMokych commented Feb 15, 2019

3\. ordered_non_unqinue replacement with oredered_unique noticed by Abit - we are in the middle of discussion now. I will experiment with Abit proposal, but I'm still not sure if it will work or not. I think we will finish with today/tomorrow.

I have recently done something similar for another project:
peerplays-network/peerplays@9946eab
Works perfectly by extending the index into a composite_key with the object_id

3\. 

Everything else is done.

Excellent news. I'll look for someone to do the review.
please ping us when you have pushed the rest of the code. Thanks

I've pushed changes for attribute_assert. Please review. Changes are in the restrictions* files.
New restriction was called attribute_assert_restriction.

Also I've pushed fix for authority validation (authority which is contained in the custom_authority).
And I've updated algorithm of authority validation. So now, either custom authority is validated or normal authority if custom one is absent.
These changes are in transaction.cpp file. Please ask any questions.

Also I've removed restriction.hpp and restriction.cpp files with old implementation of restrictions.

Today I plan to do couple of small fixes: I've found issue in API and I'll add couple of tests.
This should be ready till the end of the day.

@abitmore @xeroc @sschiessl-bcp

@sschiessl-bcp
Copy link

Today I plan to do couple of small fixes: I've found issue in API and I'll add couple of tests.

Awesome! Please ping when a new holistic review can be done

@RomaMokych
Copy link
Author

RomaMokych commented Feb 15, 2019

@sschiessl-bcp @xeroc @abitmore
Guys, you can do holistic review now. I've pushed all latest changes with fixes of authority validation and attribute_assert

@abitmore
Copy link

either custom authority is validated or normal authority if custom one is absent.

I haven't review the code yet, just comment here to confirm one thing: custom authority should be checked first, if nothing matches (but not only when "absent"), need to check normal authority.

@sschiessl-bcp
Copy link

either custom authority is validated or normal authority if custom one is absent.

I haven't review the code yet, just comment here to confirm one thing: custom authority should be checked first, if nothing matches (but not only when "absent"), need to check normal authority.

@RomaMokych ?

@RomaMokych
Copy link
Author

RomaMokych commented Feb 19, 2019

I haven't review the code yet, just comment here to confirm one thing: custom authority should be checked first, if nothing matches (but not only when "absent"), need to check normal authority.

I'll check. I think I'll need to fix this case.
For now I'm failing validation if custom_authorities present and none is matched.

Thank you @abitmore

Will update it in couple of hours. But this is small change you still can review feature.

UPD: updated it. Added tests for such case. Updated validation logic a little (transaction.cpp).

@nathanielhourt
Copy link

CMake fails to parse because libraries/chain/operation_type_to_id.cpp is missing

@RomaMokych
Copy link
Author

RomaMokych commented Feb 20, 2019

CMake fails to parse because libraries/chain/operation_type_to_id.cpp is missing

Fixed it. @nathanhourt

@xeroc
Copy link

xeroc commented Mar 13, 2019

@nathanhourt Are you taking the lead in project managing this feature? There's some mileage already done and we'd rather sprint to the finish line!

Dimfred pushed a commit that referenced this pull request Jul 1, 2019
Dimfred pushed a commit that referenced this pull request Jul 1, 2019
add unit tests for get_signed_transaction_signers, get_key_references
Dimfred pushed a commit that referenced this pull request Nov 20, 2019
Stay unchanged for agreement
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.

7 participants