-
Notifications
You must be signed in to change notification settings - Fork 86
feat: hb_device reserved keys #1077
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
base: edge
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,9 @@ | |
| -module(hb_device). | ||
| -export([truncate_args/2, message_to_fun/3, message_to_device/2]). | ||
| -export([is_direct_key_access/3, is_direct_key_access/4]). | ||
| -export([is_reserved/3, is_reserved/4]). | ||
| -export([find_exported_function/5, is_exported/4, info/2, info/3]). | ||
| -include_lib("eunit/include/eunit.hrl"). | ||
| -include("include/hb.hrl"). | ||
|
|
||
| -define(DEFAULT_DEVICE, <<"message@1.0">>). | ||
|
|
@@ -145,13 +147,15 @@ info_handler_to_fun(HandlerMap, Msg, Key, Opts) -> | |
| {ok, Exclude} -> | ||
| case lists:member(Key, Exclude) of | ||
| true -> | ||
| MsgWithoutDevice = | ||
| hb_maps:without([<<"device">>], Msg, Opts), | ||
| message_to_fun( | ||
| MsgWithoutDevice#{ <<"device">> => ?DEFAULT_DEVICE }, | ||
| Key, | ||
| Opts | ||
| ); | ||
| MsgWithDefaultDevice = | ||
| hb_maps:put(<<"device">>, ?DEFAULT_DEVICE, Msg, Opts), | ||
| {Status, _Dev, Func} = | ||
| message_to_fun( | ||
| MsgWithDefaultDevice, | ||
| Key, | ||
| Opts | ||
| ), | ||
| {Status, Func}; | ||
| false -> {add_key, hb_maps:get(func, HandlerMap, undefined, Opts)} | ||
| end; | ||
| error -> {add_key, hb_maps:get(func, HandlerMap, undefined, Opts)} | ||
|
|
@@ -226,6 +230,33 @@ is_exported(#{ exports := Exports }, Key, _Opts) -> | |
| ); | ||
| is_exported(_Info, _Key, _Opts) -> true. | ||
|
|
||
| %% @doc Determine if a key is reserved by a device. | ||
| %% A key is reserved if it is declared in the device's `info().reserved' list, | ||
| %% or if it resolves to a direct device function. Keys handled by a device's | ||
| %% default handler are not reserved. | ||
| is_reserved(Base, Key, Opts) -> | ||
| is_reserved(message_to_device(Base, Opts), Base, Key, Opts). | ||
| is_reserved(Dev, Base, Key, Opts) when is_binary(Dev) -> | ||
| DevMod = message_to_device(#{ <<"device">> => Dev }, Opts), | ||
| is_reserved(DevMod, Base, Key, Opts); | ||
| is_reserved(Dev, Base, Key, Opts) -> | ||
| Info = info(Dev, Base, Opts), | ||
| is_reserved_key(Info, Key, Opts) orelse is_device_key(Dev, Base, Key, Opts). | ||
|
|
||
| %% @doc Check if a key appears in a device's `reserved' info list. | ||
| is_reserved_key(Info, Key, Opts) -> | ||
| Reserved = hb_maps:get(reserved, Info, [], Opts), | ||
| NormReserved = lists:map(fun maybe_normalize_device_key/1, Reserved), | ||
| lists:member(maybe_normalize_device_key(Key, existing), NormReserved). | ||
|
|
||
| %% @doc Check if a key is claimed by the device's direct function dispatch. | ||
| is_device_key(Dev, Base, Key, Opts) -> | ||
| try message_to_fun(Dev, Base, Key, Opts) of | ||
| {ok, _, _} -> true; | ||
| {add_key, _, _} -> false | ||
| catch _:_:_ -> false | ||
| end. | ||
|
|
||
| %% @doc Normalize an exported key to its canonical atomized form. By default | ||
| %% new atoms are created if necessary. In practice this is used for keys that | ||
| %% orinate from a device's `info' response, but _not_ for keys that could be | ||
|
|
@@ -301,3 +332,34 @@ do_is_direct_key_access(Dev, NormKey, Opts) -> | |
| not lists:member(NormKey, Exports ++ ?MESSAGE_KEYS); | ||
| _ -> false | ||
| end. | ||
|
|
||
| explicit_reserved_key_test() -> | ||
| Trie = #{ <<"device">> => <<"trie@1.0">> }, | ||
| ?assert(is_reserved(Trie, <<"node-value">>, #{})), | ||
| ?assert(is_reserved(<<"trie@1.0">>, Trie, <<"node-value">>, #{})), | ||
| ?assert(is_reserved(Trie, <<"Node-Value">>, #{})), | ||
| ?assertNot(is_reserved(Trie, <<"alice">>, #{})). | ||
|
|
||
| message_reserved_key_test() -> | ||
| Msg = #{ <<"device">> => <<"message@1.0">> }, | ||
| Trie = #{ <<"device">> => <<"trie@1.0">> }, | ||
| ?assert(is_reserved(Msg, <<"path">>, #{})), | ||
| ?assert(is_reserved(Msg, <<"set">>, #{})), | ||
| ?assert(is_reserved(Msg, <<"get">>, #{})), | ||
| ?assert(is_reserved(Trie, <<"get">>, #{})), | ||
| ?assert(is_reserved(Trie, <<"set">>, #{})), | ||
| ?assert(is_reserved(Trie, <<"keys">>, #{})), | ||
| ?assertNot(is_reserved(Trie, <<"commit">>, #{})), | ||
| ?assertNot(is_reserved(Msg, <<"alice">>, #{})). | ||
|
|
||
| trie_keys_skip_reserved_keys_test() -> | ||
| Trie = | ||
| #{ | ||
| <<"device">> => <<"trie@1.0">>, | ||
| <<"node-value">> => ignored, | ||
| <<"get">> => ignored, | ||
| <<"set">> => ignored, | ||
| <<"keys">> => ignored, | ||
| <<"alice">> => 1 | ||
| }, | ||
| ?assertEqual([<<>>, <<"alice">>], lists:sort(hb_ao:keys(Trie, #{}))). | ||
|
Comment on lines
+336
to
+365
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its a nit but mind using |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| %%% @doc The identity device: For non-reserved keys, it simply returns a key | ||
| %%% from the message as it is found in the message's underlying Erlang map. | ||
| %%% Private keys (`priv[.*]') are not included. | ||
| %%% Reserved keys are: `id', `commitments', `committers', `keys', `path', | ||
| %%% `set', `remove', `get', and `verify'. Their function comments describe the | ||
| %%% behaviour of the device when these keys are set. | ||
| %%% Reserved keys are: `id', `commitments', `committers', `keys', `path', | ||
| %%% `set', `remove', `get', `commit', `committed', and `verify'. Their function | ||
| %%% comments describe the behaviour of the device when these keys are set. | ||
| -module(dev_message). | ||
| %%% Base AO-Core reserved keys: | ||
| -export([info/0, keys/1, keys/2]). | ||
|
|
@@ -18,7 +18,7 @@ | |
| -define(DEFAULT_ID_DEVICE, <<"httpsig@1.0">>). | ||
| -define(DEFAULT_ATT_DEVICE, <<"httpsig@1.0">>). | ||
|
|
||
| %% The list of keys that are exported by this device. | ||
| %% The list of keys that `set/3' filters before writing message data. | ||
| -define(DEVICE_KEYS, [ | ||
| <<"id">>, | ||
| <<"commitments">>, | ||
|
|
@@ -30,10 +30,18 @@ | |
| <<"verify">> | ||
| ]). | ||
|
|
||
| %% The list of keys that the message device reserves at protocol level. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this shouldn't be needed? These should all be returned as true by |
||
| -define(RESERVED_KEYS, ?DEVICE_KEYS ++ [ | ||
| <<"get">>, | ||
| <<"commit">>, | ||
| <<"committed">> | ||
| ]). | ||
|
|
||
| %% @doc Return the info for the identity device. | ||
| info() -> | ||
| #{ | ||
| default => fun dev_message:get/4 | ||
| default => fun dev_message:get/4, | ||
| reserved => ?RESERVED_KEYS | ||
| }. | ||
|
|
||
| %% @doc Generate an index page for a message, in the event that the `body' and | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,20 +24,17 @@ | |
| %%% but cannot be properly normalized. | ||
| -define(RADIX, 256). | ||
|
|
||
| %%% @doc Trie node metadata keys that must not be treated as edge labels. | ||
| -define(RESERVED_KEYS, [ | ||
| <<"node-value">>, | ||
| <<"device">>, | ||
| <<"commitments">>, | ||
| <<"priv">>, | ||
| <<"hashpath">> | ||
| ]). | ||
|
|
||
| info() -> | ||
| #{ | ||
| default => fun get/4, | ||
| reserved => ?RESERVED_KEYS | ||
| }. | ||
| reserved => [ | ||
| <<"node-value">>, | ||
| <<"device">>, | ||
| <<"commitments">>, | ||
| <<"priv">>, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't be an addressable key anyway? |
||
| <<"hashpath">> | ||
| ] | ||
| }. | ||
|
|
||
| keys(Trie, Opts) -> | ||
| collect_keys(Trie, <<>>, Opts, []). | ||
|
|
@@ -252,12 +249,12 @@ retrieve(TrieNode, Key, Opts, KeyPrefixSizeAcc) -> | |
| %% @doc Get a list of edge labels for a given trie node. | ||
| edges(TrieNode, _Opts) when not is_map(TrieNode) -> []; | ||
| edges(TrieNode, Opts) -> | ||
| Filtered = hb_maps:without( | ||
| ?RESERVED_KEYS, | ||
| TrieNode, | ||
| Opts | ||
| ), | ||
| hb_maps:keys(Filtered). | ||
| [ | ||
| Key | ||
| || | ||
| Key <- hb_maps:keys(TrieNode, Opts), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You probably want |
||
| not hb_device:is_reserved(?MODULE, TrieNode, Key, Opts) | ||
| ]. | ||
|
|
||
| %% @doc Compute the longest common binary prefix of A and B, comparing chunks of | ||
| %% N bits. | ||
|
|
||
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.
Seems OK but its a kind of strange concept when you think about it... What exactly does it mean for the key to be
reservedvsexported? We don't expect there to be any other protocol rules, but your idea is to have a way for a message's device to signal generically that while a key not be computed it... Should also not be set by the user? But it may be found as raw data because the device itself can/will set it? In which case it is part of the device's public interface -- just not computed directly?If so, I think I grok it. Comment in that case is technically correct but maybe we should add a tiny sentence explaining 'this allows device authors to signal generically to other callers that a given key in a message is anticipated to be used for the device's internal purposes, whether or not its value is defined by Erlang function calls or
message@1.0-inherited literal lookups.'?