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

add startup message to all plugins #1467

Closed
wants to merge 5 commits into from
Closed

add startup message to all plugins #1467

wants to merge 5 commits into from

Conversation

xqcool
Copy link

@xqcool xqcool commented Dec 12, 2018

add startup message to all plugins

@oxarbitrage
Copy link
Member

1- Can you explain us what you are doing and why so we don't have to try to figure out the code?
2- indentation in bitshares is 3 spaces.
3- need to be done against the develop branch.

thank you.

@xqcool
Copy link
Author

xqcool commented Dec 13, 2018

1- Can you explain us what you are doing and why so we don't have to try to figure out the code?
2- indentation in bitshares is 3 spaces.
3- need to be done against the develop branch.

thank you.

Thank you for response, ok, can you give me a code specification? and a new branch mainly used to solve issue? i just joined BTS community. This commit that i want to decrease the use of get_required_authorities

Copy link
Contributor

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

Please do not change file permissions.

_db.get_global_properties().parameters.max_authority_depth
result,
_db.get_global_properties().parameters.max_authority_depth,
1
Copy link
Contributor

Choose a reason for hiding this comment

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

We're still working on coding style guidelines. Try to format your code in a way that matches existing code.
It seems you're using tabs now instead of spaces. Please change that.

Copy link
Author

Choose a reason for hiding this comment

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

already modified,please

@@ -359,6 +359,42 @@ set<public_key_type> signed_transaction::get_required_signatures(
return result;
}

void signed_transaction::get_required_signatures(
Copy link
Contributor

Choose a reason for hiding this comment

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

Your new method is mostly a copy of existing code. Please keep your code DRY.

Copy link
Author

Choose a reason for hiding this comment

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

got it

const std::function<const authority*(account_id_type)>& get_owner,
set<public_key_type> &conti_use_reult,
uint32_t max_recursion /*= GRAPHENE_MAX_SIG_CHECK_DEPTH*/,
uint32_t need_authority_type /*= 0*/ /*0 no need ,1 need other*/
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be simpler to make conti_use_result a pointer instead of a reference, and use a null pointer to indicate that you don't want the keys.

Copy link
Author

Choose a reason for hiding this comment

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

thanks,this is a good idea

s.check_authority( get_owner( owner ) );
for( auto& active : required_active )
s.check_authority( active );
switch(need_authority_type)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a complicated switch instead of a simple if?

Copy link
Author

@xqcool xqcool Dec 14, 2018

Choose a reason for hiding this comment

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

i want to make **need_authority_type ** a enum,and swirch maybe better,but **need_authority_type ** is no more usage scenarios

@xqcool xqcool changed the title increase efficiency increase efficiency->"corvert, add startup message to all plugins" my design may not be good Dec 14, 2018
@pmconrad
Copy link
Contributor

Please create a separate PR for the startup message issue.

@xqcool xqcool changed the title increase efficiency->"corvert, add startup message to all plugins" my design may not be good add startup message to all plugins Dec 17, 2018
@oxarbitrage
Copy link
Member

There is maybe some good stuff in this pull request, honestly i cant tell. It is a pull of 30 files changes, it haves several different features on it ... plugin startup messages is totally different than increase efficiency and should be separated pulls.

I will suggest that you make smaller pull requests only with one feature at a time if you have the intention of merging code into bitshares. Probably the easier will be to close this pull request and submit new smaller one(s) that can be reviewed.

Thank you.

@oxarbitrage oxarbitrage mentioned this pull request Dec 20, 2018
34 tasks
@xqcool
Copy link
Author

xqcool commented Dec 21, 2018

There is maybe some good stuff in this pull request, honestly i cant tell. It is a pull of 30 files changes, it haves several different features on it ... plugin startup messages is totally different than increase efficiency and should be separated pulls.

I will suggest that you make smaller pull requests only with one feature at a time if you have the intention of merging code into bitshares. Probably the easier will be to close this pull request and submit new smaller one(s) that can be reviewed.

Thank you.

thanks,got it

@xqcool xqcool closed this Dec 21, 2018
@pmconrad pmconrad mentioned this pull request Dec 21, 2018
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.

None yet

4 participants