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.

Sign up for GitHub

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

Jump to bottom

Custom active permission #86

Merged
merged 89 commits into from
Aug 16, 2018
Merged

Custom active permission #86

Changes from 6 commits
Commits
Show all changes
89 commits
Select commit Hold shift + click to select a range
15fbf7f
Custom active permissions
Jul 25, 2018
7a13f4d
Update bsip-0040.md
Jul 25, 2018
aed7556
Update bsip-0040.md
Jul 25, 2018
e928c44
Update bsip-0040.md
xeroc Jul 25, 2018
e8fabfd
Update bsip-0040.md
Jul 25, 2018
5ebcbb6
Update bsip-0040.md
Jul 25, 2018
a825307
Update bsip-0040.md
Jul 25, 2018
239b7a2
Update bsip-0040.md
Jul 25, 2018
28f96ba
Update README.md
Jul 25, 2018
10fa5ad
Comments from pmconrad
Jul 27, 2018
815a50c
add more detailed asserts description and milestones
Jul 27, 2018
8591695
include more assert descrption and lisght rewriting
Jul 27, 2018
824549e
Update bsip-0040.md
Jul 27, 2018
71e57c5
Update bsip-0040.md
Jul 27, 2018
54d0ee6
Update bsip-0040.md
Jul 27, 2018
1083311
Update bsip-0040.md
Jul 27, 2018
189ceec
Update bsip-0040.md
Jul 27, 2018
cb79a4f
Update bsip-0040.md
Jul 27, 2018
e6cc5f1
Update bsip-0040.md
Jul 27, 2018
75faa36
Update bsip-0040.md
Jul 28, 2018
5b5e550
Update bsip-0040.md
Jul 28, 2018
7ae9f94
Update bsip-0040.md
Jul 28, 2018
55aa5b0
Update bsip-0040.md
Jul 28, 2018
ef397f0
Update bsip-0040.md
Jul 28, 2018
bcfa32f
Update bsip-0040.md
Jul 28, 2018
5da66a4
Update bsip-0040.md
Jul 28, 2018
6c64b50
Update bsip-0040.md
Jul 28, 2018
8207e24
Update bsip-0040.md
Jul 28, 2018
e2d9022
Update bsip-0040.md
Jul 28, 2018
fccca20
Update bsip-0040.md
Jul 28, 2018
2bcc40a
Update bsip-0040.md
Jul 28, 2018
4349e8d
Update bsip-0040.md
Jul 28, 2018
2051ff3
Update bsip-0040.md
Jul 28, 2018
6df4a38
remove length, add price as explicit allowed conversion
Jul 29, 2018
ba625b1
remove contains only, and add attibute_assert
Jul 29, 2018
1d4aa42
Update bsip-0040.md
Jul 29, 2018
523ed86
Update bsip-0040.md
Jul 29, 2018
07b6fa0
Update bsip-0040.md
Jul 29, 2018
ab5500d
attribute assert nesting
Jul 29, 2018
0e8e9ad
add economics
Jul 29, 2018
e145ede
add wording and defition of match for authority
Jul 30, 2018
d5e6f75
change dict to tuple
Jul 30, 2018
15a0514
more details on attribute_assert and where permission is stored
Jul 30, 2018
d77a526
consistent use of "list of"
Jul 30, 2018
6c7a90a
Update bsip-0040.md
Jul 30, 2018
41a2403
Update bsip-0040.md
Jul 30, 2018
cf72eaa
add logical assert
Jul 30, 2018
f0d16bb
Update bsip-0040.md
Jul 30, 2018
63f6ddc
Include either or example
Jul 30, 2018
b7282a5
fix formatting of new example
Jul 30, 2018
9b07069
include milestone differentation
Jul 30, 2018
12355d6
clarify milestone procedure
Jul 30, 2018
2222cea
moved examples out of specifications
Jul 30, 2018
b1864f8
moving around
Jul 30, 2018
03f24f0
more cleaning up
Jul 30, 2018
8dce372
change typos
Aug 2, 2018
34d0086
clarify month call
Aug 2, 2018
651e00c
Update bsip-0040.md
Aug 2, 2018
8893727
Update bsip-0040.md
Aug 2, 2018
5d92967
add enabled flag
Aug 2, 2018
b8f2691
include new comments
Aug 3, 2018
871b93d
more detail for Example: Simple transfer
Aug 3, 2018
d036c8c
calrify child operations of proposals
Aug 3, 2018
ecf22d4
add reviewers
Aug 3, 2018
7d1468e
updated Outline of handling incoming transactions
Aug 3, 2018
d9466f2
Update bsip-0040.md
Aug 3, 2018
5ddc6cd
Update bsip-0040.md
Aug 5, 2018
40b7ae4
proposals can be created by anyone
Aug 7, 2018
552b5c4
include multi-sig example
Aug 7, 2018
6a00164
Update bsip-0040.md
Aug 8, 2018
5dcf316
Update bsip-0040.md
Aug 9, 2018
3ac2159
update economics
Aug 9, 2018
f1f9755
add length and eq to number comparison
Aug 9, 2018
0919eae
Update bsip-0040.md
Aug 9, 2018
78100ed
add `contains, not_contains`
Aug 9, 2018
a1ce8ab
Update bsip-0040.md
Aug 9, 2018
e9c8e9b
add neq
Aug 10, 2018
f9c2df5
Clarify duration
Aug 10, 2018
bd2af37
If beneficial for performance or complexity sections
Aug 10, 2018
0c43bfd
Update bsip-0040.md
Aug 10, 2018
0f2a433
Update bsip-0040.md
Aug 10, 2018
982348c
add `remaining_executions`
Aug 10, 2018
64338a4
Update bsip-0040.md
Aug 10, 2018
02bb9e5
Milestone 4 in separate section
Aug 10, 2018
1d0a341
Update bsip-0040.md
Aug 10, 2018
e749319
Update bsip-0040.md
Aug 10, 2018
51d46da
Update bsip-0040.md
Aug 12, 2018
b560291
Update bsip-0040.md
Aug 13, 2018
b0dfaa5
Update bsip-0040.md
Aug 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions bsip-0040.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ List of possible restrictions are:
| ------------- |:-------------:| -----:|
| `any` | [`list`, `of`, `allowed`, `values`] | stateless |
| `none` | [`none`, `of`, `these`, `values`] | stateless |
| `lt, le, gt, ge` | `comparative` | stateless |
| `lt, le, gt, ge, eq` | `comparative` | stateless |
Copy link
Member

Choose a reason for hiding this comment

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

We already added eq, why not add ne aka "not equal"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added neq

| `contains, not_contains` | [`list`, `of`, `values`] | stateless |
| `limit` | [`max_cumsum`, `interval_in_sec`] | [`current_cumsum`, `interval_began`] |
| `limit_monthly` | [`max_cumsum`, `interval_in_months`] | [`current_cumsum`, `interval_began`] |
Copy link
Member

Choose a reason for hiding this comment

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

Best to have an function "can_not_change" for update-like operations.

For example, we may want to use a bot to update an asset's core exchange rate via asset_update_operation, but don't want it to be able to update other data fields e.g. asset description or total supply.

Actually this can be a bit hard to define, because some data fields in some operations are nested, for example, core_exchange_rate is not an attribute of asset_update_operation, but is an attribute of options field of asset_update_operation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added a contains_only assert, which flips your logic around. A custom active authority is more likely to allow just one specific attribute rather than everything except one attribute.

Copy link
Member

Choose a reason for hiding this comment

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

As I've stated, this part is hard.

  1. fields need to be standardized, otherwise the back end won't understand. That said, we need to define mapping rules for fields of each operation, primarily due to existence of nested structures, e.g. price or asset_options.

  • price is a 2-level struct with 4 data fields: {"base":{"amount":1,"asset_id":"1.3.1"},"quote":{"amount":2,"asset_id":"1.3.2"}}.

    • by the way, usually a price need to be validated as a whole, e.g. we need to assert base.amount/quote.amount < x.

  • asset_options is a nested struct as well, it even has a price object inside.

    Actually, we need to define format of "argument" as well. When the field type is static_variant, it would be hard to define.

  1. An operation have to contain all the needed fields. My example above is that the values in the incoming transaction need to be same as current values on chain. But when creating the asserts (rules) we don't know the exact value or don't care what the value is when processing a transaction, so can't put a value there.

A "contains_only" assert makes some sense, but it's operation-specific:

  • for operations like "account_update" or "asset_update" we need to check whether the values are same as current on-chain values;
  • for operations like "call_order_update" or "account_whitelist" we need to check whether the values are zero or empty, because they're deltas;
  • for operations like "transfer" perhaps we need to check whether the memo is empty if it's not in the list;
  • for operations like "limit_order_create" it seems there is nothing to check so far.

Last, not like other functions which each applies to an argument/field, this "contains_only" function applies to the whole operation?

Copy link
Collaborator Author

@sschiessl-bcp sschiessl-bcp Jul 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

  1. The lt asserts (and similar) have now an explicit type conversion for price, others may be added as well. This might be unnecessary considering the new attribute_assert. I have also included a line that argument type can be added to assert objects if necessary. I want to avoid operation specific logic in the custom active authority check at all costs.

  2. On-chain state checking should not be possible (would mean that we need to know which operation store what and where on chain). In general: If there is no assert defined, any value is allowed.

  3. contains_only does no longer exist as on chain state checking should be avoided. I have introduced attribute_assert. We can define a new assert like you originally suggested or re-introduce contains_only. I want to avoid any operation specific logic, this dropped atm

Copy link
Member

Choose a reason for hiding this comment

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

I discussed this with Stefan already. Nested objects are probably out of scope here.
In case of voting (as an example), we would propose a new operation that can only vote.
I suggest we do the same with core exchange rate ..

| `attribute_assert` | list of restrictions | stateless |
Expand Down Expand Up @@ -130,12 +131,20 @@ Stateless assert, all argument types. `Argument value` must NOT be equal to any
Stateless assert. Allows explicit type conversion:
- `int` type: use as is
- `string` type: use `length` of string as `argument value`
- `object` type: use `size` of object as `argument value`
- `list` type: use `length` of the list as `argument value`

The different asserts read as:
- `lt`: `Argument value` must be less than `comparative`
- `le`: `Argument value` must be less than or equal to `comparative`
- `gt`: `Argument value` must be greater than `comparative`
- `ge`: `Argument value` must be greater than or equal to `comparative`
- `eq`: `Argument value` must equal to `comparative`

#### `contains, not_contains`
Stateless assert, for `list` type arguments.
- `contains`: The `argument value` must contain all items specified by `data`, but can contain more
- `not_contains`: The `argument value` must NOT contain any of the items specified by `data`, but can contain others
Copy link
Member

Choose a reason for hiding this comment

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

TBH I don't like to use a list as parameter of a function named contains, because it's not common. A common contains function (or shorter name "has") means "A is a list, B is in the list". In some programming libs there are "contains_all" functions which fit your design better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have changed the wording to contains_all and contains_none. Single object contains is a special case of list-based contains, I added that that can be added if beneficial for performance.


#### `limit`
Stateful assert, only `int` type arguments. When the authority is created, `interval_began` is set to `valid_from` from its custom active authority and `max_cumsum` to `0`. Incoming operations are first tried to match all stateless asserts,
Expand All @@ -158,10 +167,10 @@ When a signed transaction arrives and before the backend evaluates if all necess
- iterate over `required accounts`
- iterate over all `operations` (child operations of proposal are not included) within the transactions that require the active authority of this account
- iterate the `custom_active_authorities` of this account, and if it matches, remember that and continue with next `operation`
- if a `custom active authority` match was found for every operation in the loop, behave as if the `active authority` of this account is present for the correspoding operations
- if a `custom active authority` match was found for every operation in the loop, behave as if the `active authority` of this account is present for the corresponding operations

Note:
- A `custom_active_authority` can only grant the `active authority` of the corresponding account for the matching operation, nothing more
- A `custom_active_authority` can only grant the `active authority` of the required account of the corresponding operation, nothing more
- The actual active authority of a required account can still be given as before, existing behavior is not to be changed
- This for illustration, not actual implementation instructions

Expand All @@ -171,9 +180,13 @@ Adding a custom active authority means increased effort for the backend, and wit
- `install_custom_active_authority`: Normal accounts can only create custom active authoritites with a duration of maximum 1 year, LTM can do any duration. Two options come to mind:
- A fixed high fee independent of authorities content
- Tied to the duration and complexity of the custom active authority. Transaction fee is then `fee = flat_fee + basic_fee * duration` where `basic_fee` is calculated according to complexity (e.g. size of authority, number of restrictions and etc.). Fee is capped at 1 year for LTM.
- `update_custom_active_authority`: Base fee similar to `account_update` plus dynamic one for any duration changes
- `update_custom_active_authority`: Base fee similar to `account_update` plus dynamic one for any duration changes, no payback if duration in decreased.
- `delete_custom_active_authority`: Cheap similar to `limit_order_cancel`

This logic forces the user to think about the desired duration and enforces to put it rather too short than too long.
If a custom active is expired, or considered to be too short, extending it is easily doable.
Copy link
Member

Choose a reason for hiding this comment

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

How about a user changing period from 2018-08-01 - 2018-09-01 to 2018-08-15 - 2018-09-15 on August 20th? The duration doesn't increase, but it effectively extends.

Copy link
Collaborator Author

@sschiessl-bcp sschiessl-bcp Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I see. It was meant like duration refers to the time that is still left, not the actual duration (i.e. date_to - max(now, date_from)).

In your example, the reference base duration when updating id 2018-08-01 - 2018-09-01. So updating to 2018-08-15 - 2018-09-15 means adding 14 days.

Copy link
Member

@abitmore abitmore Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

now is chain state which can't be used when calculating fee with current design principle. Unless we add a suppose_now field in the operation and evaluate it against chain state, but it would be ugly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, good point. The fee would then depend on execution time and not creation time.

Let's simply do it that all time period increase counts, whereas decrease is ignored.

In your example:
The change 2018-08-01 to 2018-08-15 decreases the period, ignore.
THe change 2018-09-01 to 2018-09-15 increases the period, add to fee.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately the value before change is in chain state so can't be used as well.

Copy link
Member

Choose a reason for hiding this comment

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

Then the fee of updating should take argument size and authority size into account, but not only duration difference. Ideally,

update_fee = basic_fee + max( 0, total_new_fee - unused_old_fee )
           = basic_fee + max( 0, 
                new_fee_rate * new_duration * new_auth_size * new_argument_size
              - old_fee_rate * max(old_time_to-now, 0) * old_auth_size * old_argument_size )

Since all old data is in chain state, to calculate the difference, we need to put some "supposed" data in the operation, to be simpler, we can directly put the supposed delta in the operation and calculate in background.

Copy link
Collaborator Author

@sschiessl-bcp sschiessl-bcp Aug 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Note: I changed format of restriction a bit (mentioned in #86 (comment)),

You basically added a modifier field, which is used to extract the implicit type conversions into explicit ones. Which is totally fine and is covered by my note in the BSIP
"This list of restrictions may be redefined to improve clarity, performance or to reduce complexity of implementation while maintaining the intended functionality"

Since all old data is in chain state, to calculate the difference, we need to put some "supposed" data in the operation, to be simpler, we can directly put the supposed delta in the operation and calculate in background.

What is best here for implementation I leave to you (the devs). One question I have: If the user overpays the transaction fee, will he (or can he) get the rest back?

Copy link
Member

Choose a reason for hiding this comment

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

One question I have: If the user overpays the transaction fee, will he (or can he) get the rest back?

In BitShares fees can always be overpaid, and the system never refund overpaid fees except the "refund order creation fee when order is cancelled" feature.

Copy link
Member

Choose a reason for hiding this comment

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

BTW I mentioned fee pooling mechanism earlier but Peter didn't like it at this early stage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I saw that, but that takes it to a new level in terms of complexity. I think user can be expected to think before they create one.

After expired, the custom active becomes disabled and can still be enabled again with a new period, paying the fee for that new period.

### Modification to the backend

* Add a new index or extend the account object to store custom active permission are assigned to an account and contain a list of custom active authorities. Multiple custom active authority entries are possible for one operation
Expand Down Expand Up @@ -360,7 +373,7 @@ the transaction is executed.

We propose do split the implmentation into two milestones. Each milestone will be voted on as a separate BSIP:
Copy link
Contributor

Choose a reason for hiding this comment

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

You list three milestones, not two

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Not done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now done


1. Implementation of basic functionaliy to allow custom active permissions and authorities, including `any`, `none` and `lt, le, gt, ge` and `attribute_assert` `asserts`. If deemed necessary by developpers, reduce to only allow one key or one account for every `custom active authority`
1. Implementation of basic functionaliy to allow custom active permissions and authorities, including `any`, `none` and `lt, le, gt, ge`, `contains, not_contains` and `attribute_assert` `asserts`. If deemed necessary by developpers, reduce to only allow one key or one account for every `custom active authority`
2. Implement stateful asserts `limit` and `limit_monthly`
3. Implement `logical_or`

Expand Down