-
Notifications
You must be signed in to change notification settings - Fork 649
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
BSIP87 Tests #2172
BSIP87 Tests #2172
Conversation
This reverts commit 0a789cf.
One test still fails due to phrasing of insufficent accumulated fees exception.
There are commits
Please rebase and remove these commits from the history. Thanks. |
OK, I changed my mind. The 2 commits are quite small, but it might need quite some efforts to remove them, I think it's fine to keep them, although generally it's not perfect. |
const string &name, | ||
account_id_type issuer /* = GRAPHENE_WITNESS_ACCOUNT */, | ||
uint16_t force_settlement_offset_percent /* 100 = 1% */, | ||
optional<uint16_t> force_settlement_fee_percent /* 100 = 1% */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we define a default value for this argument, then the function above with one argument fewer can be removed. Current code is acceptable for test cases though.
BTW I think it's better to add new parameters to the functions in database_fixture
class, rather than adding new functions here. It's not a priority though, we can refactor in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are reasonable points. We'll defer the refactoring for now.
Thanks for the helpful review @abitmore. Hopefully the revisions address all of your comments. |
…aiming collateral-denominated fees
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Unit tests followup to #2151.