search

iotaledger / access-server

Apache License 2.0
14 stars 3 forks source link

Refactor APIs + Plugins #102

Closed bernardoaraujor closed 4 years ago

bernardoaraujor commented 4 years ago

Placeholder issue to centralize discussions around the refactoring activities. Now that the sprints are over, I want to go through the code and apply some additional changes to increase the modularity of the APIs and Plugins, as well as to better understand some aspects of the implementation.

bernardoaraujor commented 4 years ago

@strahinjagolic could you explain the motivation behind the Authority introduced into the PIP module? Also, how it relates to the uri argument taken in by PIP_get_data.

bernardoaraujor commented 4 years ago

@strahinjagolic + @djordjeglbvc can you guys review this diff and let me know whether it makes sense to you? https://pastebin.com/FPB9rfA5

The motivation behind it is that I want to avoid API calls leaking outside of their scope as much as possible.

With these changes, I'm making sure that the choice of Resolver Plugin is made in the definition of main.c, which falls into the scope of Reference Implementation.

Then, PEP_register_callback() is called inside the scope of access.c (SDK) while being agnostic to which kind of resolver is being registered.

Driven by the same motivation I want to soon move the CAN/OBDII/etc stuff away from access.c (SDK) and into main.c (RI). But that's probably another discussion.

ps.: don't worry about applying these changes or resolving conflicts with the branches you're currently working on. I'm working on a private fork just to experiment with some ideas and get feedback from you guys here.

strahinjagolic commented 4 years ago

@bernardoaraujor

could you explain the motivation behind the Authority introduced into the PIP module?

This is explained in specs.

In theory, we may have other wallets than IOTA, or at least we can use IOTA mainnet or IOTA testnet. Each of them can have different plugin (in specs, it's faulty called resolver), so I've added options to support this, but for now it is hardcoded to IOTA.

Also, how it relates to the uri argument taken in by PIP_get_data.

The format of uri looks like: authority:policy_id/type?value

strahinjagolic commented 4 years ago

@bernardoaraujor

The motivation behind it is that I want to avoid API calls leaking outside of their scope as much as possible.

I fully support this, this is a very good idea. However, as I can see in the diff, there is still a link between wallet and access actor, which is not designed in architecture. If you are going in this direction, maybe you should remove this.

Driven by the same motivation I want to soon move the CAN/OBDII/etc stuff away from access.c (SDK) and into main.c (RI).

Communication protocols should be refactored completely, but this is a LARGE effort.

djordjeglbvc commented 4 years ago

@strahinjagolic + @djordjeglbvc can you guys review this diff and let me know whether it makes sense to you? https://pastebin.com/FPB9rfA5 The motivation behind it is that I want to avoid API calls leaking outside of their scope as much as possible.

With these changes, I'm making sure that the choice of Resolver Plugin is made in the definition of main.c, which falls into the scope of Reference Implementation.

Then, PEP_register_callback() is called inside the scope of access.c (SDK) while being agnostic to which kind of resolver is being registered.

Driven by the same motivation I want to soon move the CAN/OBDII/etc stuff away from access.c (SDK) and into main.c (RI). But that's probably another discussion.

ps.: don't worry about applying these changes or resolving conflicts with the branches you're currently working on. I'm working on a private fork just to experiment with some ideas and get feedback from you guys here.

I absolutely agree that they should be moved out of the access sdk, both resolver and data acquisition plugins, as currently we have a spaghetti situation. So both registration of plugins and implementations. Just have in mind that datasets go in tandem with data acquisition plugins (can, obdii, etc), in a sense that when implementing the plugin, you create dataset specifically for it. So dataset implementations should also be moved away from access sdk.

bernardoaraujor commented 4 years ago

I fully support this, this is a very good idea. However, as I can see in the diff, there is still a link between wallet and access actor, which is not designed in architecture. If you are going in this direction, maybe you should remove this.

Yes I'm aware, that was just a little experiment that I wanted to get the feedback from you guys and illustrate some concepts.

Communication protocols should be refactored completely, but this is a LARGE effort.

I agree, let's start to slowly think about how to do that.

(in specs, it's faulty called resolver)

I see why you would say it's faulty to call it resolver in the PIP context, because it conflicts with the resolver convention we have in place for PEP context.

But when I take a step back and reflect on it, I feel like resolver is actually much more appropriate in this PIP context than PEP, especially if we're dealing with URIs.

This brings me to the question: how to deal with non-DLT authorities? I mean, PIP will be used to check transactions, but it's function is much more generic and can also take sensor data for example. How would a sensor URI look like?

Correct me if I'm wrong, but I get the sense that PIP sensor functionality has not been established in the codebase yet, and data acquisition implementation has only been used in PEP context for data sharing. I must admit I don't fully understand how PEP is sharing data with the client. Can you guys elaborate a scenario where I'll be able to visualize this?

Anyways, we should come up with a strategy to improve the plugin naming scheme and directory hierarchy, because it's a bit confusing the way it is now. Especially after (re)reading this hackmd specification, where resolver is originally used with a different purpose compared to what it is now.

bernardoaraujor commented 4 years ago

Edit: If I allow myself to diverge a bit from the current terminology, I would propose the following URI schema:

But that's assuming I understand the hackmd specification correctly, so please correct me if I'm missing something.

Ant there's also @strahinjagolic 's version which looks like: authority:policy_id/type?value

here protocol and authority are probably playing the same role, but I have a hard time to differentiate between policy_id and type.

bernardoaraujor commented 4 years ago

In theory, we may have other wallets than IOTA, or at least we can use IOTA mainnet or IOTA testnet.

@strahinjagolic that's not correct. The choice between mainnet or testnet is done solely on the definition of NODE_URL, so it doesn't have any impact in relationship to PIP authority.

bernardoaraujor commented 4 years ago

@strahinjagolic + @djordjeglbvc + @vlad-ns I created a dummy PR to expand on the ideas I was exploring in the patch I sent yesterday.

https://github.com/iotaledger/access/pull/107

Please note that I'm not only isolating pep_register_callback into access.c (SDK), but I'm also allowing PEP to handle multiple callbacks (similar to how PIP allows for multiple as well).

The implementation is buggy (segfault when I execute) and I wasn't able to come up with a clever way to calculate the index inside pep_request_access(). But I think it illustrates a few points.

Please take a look and let me know what you guys think.

strahinjagolic commented 4 years ago

@bernardoaraujor

But when I take a step back and reflect on it, I feel like resolver is actually much more appropriate in this PIP context than PEP, especially if we're dealing with URIs.

Why?

This brings me to the question: how to deal with non-DLT authorities? I mean, PIP will be used to check transactions, but it's function is much more generic and can also take sensor data for example. How would a sensor URI look like?

I wouldn't separate sensor functionality of the PIP from DLT, since the whole idea is DLT-based in the first place. E.g. - You have User1 who wants to use embedded device. User1 will pay for services and during the policy check, both, payment and sensor data conditioning this service, will be checked by the PIP. Thus, URI is universal. For sensor information, format is the same: iota:policyid/type?value, where type is e.g. request.tyrePressure.type and value will be request.tyrePressure.value. So after we get sensor value, we will fill attribute object in PIP with two strings -> type = "PSI", vale = "30".

Correct me if I'm wrong, but I get the sense that PIP sensor functionality has not been established in the codebase yet, and data acquisition implementation has only been used in PEP context for data sharing. I must admit I don't fully understand how PEP is sharing data with the client. Can you guys elaborate a scenario where I'll be able to visualize this?

It is established in PIP, but the problem is in data acquisition plugin. Data acquisition module should register the callback in protocol.c, but this is still not elaborated thoroughly, that's why I mentioned that data acquisition modules should be refactored and it is a huge job. Regarding data sharing - this is not, actually, done by the PIP. The data sharing request is simple action request, same as 'open the door' or 'blink lights'. After this action is requested, resolver will set the dataset that needs to be shared to data acquisition plugin and it will start the acquisition. Data acquisition modules are storing the data in JSON file and (when it's activated) they are using a socket to send this data over TCP.

I would propose the following URI schema: protocol/type?value

Base idea is that only one protocol shall be used for acquiring the data. From that perspective, it's no use to introduce "protocol" designator. However, if we refactor data acquisition modules, we can probably manage to pull this out. We should consider this option. Regarding my URI schema (iota:policyid/action?value) - as mentioned above, I wouldn't separate authority from sensor functionality, so, I would leave this authority. Also, policy id is used to verify transaction, so it must stay also. Policy ID and the type are two completely different thing. Policy ID is hash of the policy object and it's used to describe subject-action pair unambiguously. Type is type of the information that is requested, e.g. "km/h", "min", "kg" or even "string" (like for values "paid", "not_paid", "verified").

The choice between mainnet or testnet is done solely on the definition of NODE_URL, so it doesn't have any impact in relationship to PIP authority

It's just an example.

Please take a look and let me know what you guys think.

Can you explain the idea wit multiple PEP clients? I'm not sure that PEP should be aware of the actuators. In ideal case, PEP should only take request from access client, verify request through the PDP and call actuator interface, which is independent and shall provide connection to real actuator driver (which will be platform depended, of course). Also, where do you plan to initialize plugins?

bernardoaraujor commented 4 years ago

@strahinjagolic

Why?

Because linguistically speaking, "resolving" a URI makes more sense than "resolving" an action. IMO it feels more intuitive, but this is highly subjective and just a small remark, not really a big deal atm.

In relationship to the concept of authority/protocol, I think the fact that the initial PoC was based on Ethereum has the potential to become a bit misleading, so I would like to emphasize the following:

Like I mentioned before, the choice of network (mainnet/testnet/privatenet) is done when NODE_URL is defined, so it doesn't make much sense to think about encoding which network is being used in the URI. It suffices to say that we're dealing with the IOTA protocol, and that's all. So I feel the keywork authority starts to lose its meaning in a sense.

That's why I felt it would make sense to switch keyword authority for protocol, so it helps answer the questions: what kind of attribure is this? Is it the state of a GPIO port? Is it data coming from a sensor via CAN bus? Or is it some transaction stored in the Tangle?

That way, we could have many protocol choices, like iota, gpio, can, gps, modbus, obdii, and so on. And instead of having callback_fetch[authority], we would have each callback_fetch[protocol] fetching data from its respective callback implementation that follows the proper protocol.

For example:

But that brings me to your statement:

Base idea is that only one protocol shall be used for acquiring the data.

Can you elaborate on what you mean by protocol here? Cause perhaps we might be using the same term with slightly different definitions.

Can you explain the idea wit multiple PEP clients?

Modularity.

If there's room for only one resolver plugin (PEP client == resolver plugin, right?), we are forcing developers to reimplement the same thing over and over again. That is not ideal.

Take the Wallet resolver plugin for example. We don't want its functionality to be reimplemented every time there's Tangle interactions.

Also, where do you plan to initialize plugins?

Please check https://github.com/iotaledger/access/pull/107/files:

In this PR I'm illustrating the simplified use-case I want the initial Reference Implementation to be. Note how only Relay and Wallet plugins are initialized, but they are separated from each other.

djordjeglbvc commented 4 years ago

@bernardoaraujor

@strahinjagolic + @djordjeglbvc + @vlad-ns I created a dummy PR to expand on the ideas I was exploring in the patch I sent yesterday.

107

... Please take a look and let me know what you guys think.

After a quick glance it generally looks like something I wanted to do. Regarding access_ctxt structure, I'd make ddstate field generic, or make it an array of void pointers, because different plugins would use different ddstate structures, and access agnostic towards them. So plugins would be responsible in managing their own dataset state objects (alloc, free, etc). Or even better - move ddstate object completely from access to the context of plugin instance or PEP instance, whichever makes more sense.

strahinjagolic commented 4 years ago

@bernardoaraujor

Can you elaborate on what you mean by protocol here? Cause perhaps we might be using the same term with slightly different definitions.

By "protocol", I mean communication protocol (CAN, GPIO, SPI...). We can start all supported protocols and use URI to choose one of them, but, as I sad, this requires a lot of work. I don't mind this solution, but we must plan great amount of time for this. Therefore, we can remove "authority" keyword, but URI should steel have "policy_id" field.

If there's room for only one resolver plugin (PEP client == resolver plugin, right?), we are forcing developers to reimplement the same thing over and over again. That is not ideal.

Understood! I agree with this.

In this PR I'm illustrating the simplified use-case I want the initial Reference Implementation to be

And what about, e.g. Data plugin, would it be initialized in similar manner?

bernardoaraujor commented 4 years ago

And what about, e.g. Data plugin, would it be initialized in similar manner?

@strahinjagolic yes, I'm working on some additional changes, I'll update the PR soon.

Regarding access_ctxt structure, I'd make ddstate field generic, or make it an array of void pointers, because different plugins would use different ddstate structures, and access agnostic towards them. So plugins would be responsible in managing their own dataset state objects (alloc, free, etc). Or even better - move ddstate object completely from access to the context of plugin instance or PEP instance, whichever makes more sense.

@djordjeglbvc cool I'll try to make something in that direction.

bernardoaraujor commented 4 years ago

@strahinjagolic + @djordjeglbvc please check https://github.com/iotaledger/access/commit/011c581624c5aa28d8e301fb83d51c64b3f4a4fc

not related to Data Acquisition yet, but something I wanted to illustrate as well.

note how with this approach, we're removing the need for wallet_ctx_t representations outside the scope of the actual wallet-based plugin.

of course, the PIP-related stuff haven't been touched yet, so that only applies to PEP context... but that's the direction I want to start walking towards

djordjeglbvc commented 4 years ago

@bernardoaraujor I would maybe leave void* options argument there, to have option for user to pass some plugin specific data from context in which it is created, eg from main.c. Thus plugin api remains generic and extendable.

bernardoaraujor commented 4 years ago

@djordjeglbvc I understand your point, and it's something I also considered. Although I would be in favor of making resolver plugin API extendable, I prefer to keep it as simple as possible for now, so I ended up deciding on removing options for a few reasons. :

1 - If we introduce options, then we also introduce the need for these options being injected into access_init() context as arguments, which I feel is not ideal. I eventually want to have access_init() signature looking something like this:

access_ctx_t access_init(probe_plugin_initializer_t probe_init_fn[], resolver_plugin_initializer_t resolver_init_fn[])

where probe plugin just means the PIP equivalent of the PEP resolver plugins. I'm using probe in this example cause I feel the word expresses better than fetch and acquire, but those have also been used with similar meanings in the code. In a general sense, it's Data Acquisition plugins for PIP, but there's some implications related to the need for refactoring those as @strahinjagolic mentioned.

2 - If we introduce options, this loop of access_init is no longer feasible, because each instance of resolver_init_fn[i] would require different args. Each resolver callback would need to be explicitly instantiated. I did it this way to keep access.c agnostic:

  for (int i = 0; i < PEP_MAX_ACT_CALLBACKS; i++){
    pep_register_callback(i, resolver_init(resolver_init_fn[i], &ctx->ddstate));
  }
strahinjagolic commented 4 years ago

@bernardoaraujor Seems like a good idea, but keep in mind that PIP (or data plugin) should also be aware of the wallet context for the transaction validation purpose.

bernardoaraujor commented 4 years ago

@strahinjagolic yes I'm aware, I'm preparing a new commit with the same changes on PIP context.

bernardoaraujor commented 4 years ago

@strahinjagolic + @djordjeglbvc + @vlad-ns

I'm renaming:

https://github.com/iotaledger/access/pull/107/commits/e940710158d7fd85e7769f580b75c8b4b6ad161c is quite extensive. I added new PIP plugins for GPIO and Wallet, although there's a few empty bits where implementation still needs to be done (for example, line 52 of pip_plugin.c ).

@strahinjagolic this is the direction we should take PIP. GPIO and Wallet are two minimalistic examples. I felt the implementations on protocol.c and transaction.c were getting too confusing, and this is probably a simpler way to achieve similar results. Please let me know what you think.

Note that now, there's no need for representation of wallet_ctx inside access_ctx anymore. That makes things much cleaner and avoids API leaks. Also, PIP, PEP, PAP (and PDP indirectly) are all initialized from access_init(), which is also the ideal scenario in terms of avoiding spaghetti.

One detail that I did not take care of is how transactions are stored locally. That's some functionality implemented in transaction.c but I feel it's best it resides somewhere inside pip_plugin directory (although I'm still not sure where).

Overall, the implementation in this PR builds but doesn't execute. I get a segmentation fault when I try to run it, as well as some exotic warnings on the build.

If we eventually decide to merge this PR, we need to debug the code.

However, my original intention was to use this PR just as a way to illustrate some ideas, and it's probably best that @strahinjagolic and @djordjeglbvc reimplement these changes with the proper attention as I'm sure both of them are much more experienced programmers than me.

djordjeglbvc commented 4 years ago

I'd just add few things in regards to plugins in general. I'll use pip plugin in e940710158d7fd85e7769f580b75c8b4b6ad161c as an example, but it applies to all of them.

We have pip_plugin_t structure, which holds all plugin callbacks. This structure should be passed as an argument to all plugin API calls, but it isn't used anywhere. Instead, we are passing callbacks around, which is very bad practice. For instance, pip_plugin_init and pip_plugin_term should look something like this:

int pip_plugin_init(pip_plugin_t *plugin, pip_plugin_initializer_t initializer);
int pip_plugin_term(pip_plugin_t *plugin);

where initializer is function which initializes plugin structure with callbacks and ddstate. This makes things neat. pip_plugin_term would then just call plugin->term_cb(plugin), which frees ddstate among other plugin specific things. As ddstate would be void* field in plugin, initializer would create new plugin specific ddstate object there, while leaving API agnostic towards its semantics. For each callback in plugin structure, there should be API call, for instance pip_plugin_start, pip_plugin_stop, etc, and they would just call callbacks like term example.

Regarding actual plugin implementation, user would implement callback functions (which can be static), ddstate structure (also doesn't need to be visible outside of plugin's context), and initializer function which initializes pip_plugin_t structure with callbacks and ddstate. So only initializer would need to be visible outside of plugin's c file. All of the plugins functionality would be called through pip_plugin api calls, passing pointer to pip_plugin_t structure.

Sorry for much repeated info in this comment, I'm never sure how bad my explaining is.

strahinjagolic commented 4 years ago

Agree with @djordjeglbvc. Also, why do you need data sharing callbacks in PIP plugin?

bernardoaraujor commented 4 years ago

@djordjeglbvc those are very interesting insights and I think they will make things even better.

@strahinjagolic I was copying PEP and wasn't sure about including them or not, so in doubt I included. But it probably makes sense to remove them.

I will spend a few more hours during the weekend introducing these changes and I will discuss with @vlad-ns how to proceed from there.

The two options I have in my head are:

djordjeglbvc commented 4 years ago

@bernardoaraujor not a good idea to consciously introduce new problems. We should strive to leave a code in a better state than we found it. Let's merge it when it is completed.

bernardoaraujor commented 4 years ago

@djordjeglbvc @strahinjagolic @vlad-ns @oopsmonk

I'm in an improvised setup, just realized the video capture was faulty. sorry I'll record again.

bernardoaraujor commented 4 years ago

@strahinjagolic here's the video (sorry again) https://drive.google.com/file/d/1Hf_TYVBS7Rs2Or6cnf1JnHKEO9fOlHov/view?usp=sharing

pay special attention to commit b27ec7cb020e78c12cba61fb7087a969163d4ae5 this is likely where some nasty bugs are being introduced with that ugly for loop and empty callback slots

on 011c581624c5aa28d8e301fb83d51c64b3f4a4fc is created and destroyed in the context where its used, and wallet define statements are moved into wallet.h (avoiding spaghetti)

bernardoaraujor commented 4 years ago

moving discussion to https://github.com/iotaledger/access/issues/110