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

Expose Forwarded Actions to Target Apps before Execution #314

Open
wants to merge 43 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@topocount
Copy link

commented May 27, 2019

Changes

This PR implements an interface that:

  1. Allows Forwarders to log forwarded actions that were caught and update them as they are either approved or declined: see newForwardedAction and updateForwardedAction
  2. Allows target apps to track the progress of forwarded actions logged by Forwarders and handle them accordingly in script.js and other webWorker files for the purpose of informing the user of relevant activity elsewhere: see getForwardedActions

newForwardedAction and updateForwardedAction are really syntactic sugar on top of the setForwardedAction wrapper method.

getForwardedActions is intended to be implemented similarly to an external contract event subscription in script.js. The key difference though is that, rather than just emitting a single action on each change. All forwarded actions targetting the subscribed app and their present status are emitted to the app. The idea behind this is that all state can be naively updated after one change, as opposed to the app needing to completely rebuild state using events each time it loads. Because forwarded actions are essentially managed by the wrapper, it makes sense that the wrapper also serves as a single source of truth for forwarded action state, as opposed to invidual apps each compiling it's own state.

If you are modifying the external API of one of the modules, please remember to also change the documentation

  • I have updated the associated documentation with my changes

Quazia and others added some commits May 18, 2019

Quazia
@CLAassistant

This comment has been minimized.

Copy link

commented May 27, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
3 out of 4 committers have signed the CLA.

topocount
chadoh
rkzel
Quazia


Quazia seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

topocount and others added some commits May 27, 2019

Merge remote-tracking branch 'upstream/master'
* upstream/master:
  react-api v2.0.0-beta.4
  @aragon/wrapper v5.0.0-rc.11
  api: v2.0.0-beta.4
  Add handler for searching local identities  (#308)
  App state caching and loading states (#297)
  Added probot bots (#319)
  @aragon/wrapper: 5.0.0-rc.9
  wrapper: use longer throttleTime instead of shorter debounceTime to throttle installedRepo observable (#315)
  @aragon/rpc-messenger: 2.0.0-beta.1

@2color 2color requested a review from sohkai Jun 17, 2019

topocount added some commits Jun 18, 2019

Deconstruct input params from messenger
- decontruct params when passing from wrapper to messenger
- remove "from" param from the API since the proxy address is utilized

@topocount topocount force-pushed the AutarkLabs:master branch from f1a8b75 to c02f9e0 Jun 25, 2019

topocount added some commits Jun 25, 2019

@sohkai
Copy link
Member

left a comment

First of all, thanks for writing the tests and documentation!

There are a bunch of comments, with most of them fairly simple comment suggestions / organization / etc.

However, there are some major points to discuss:

  • It would be great to move out some of the unrelated items (e.g. membership, tokens, etc.) to a different branch / PR
    • It's unclear if the metadata APIs are ready yet or not
  • Forwarding actions
    • We need to take into account what happens when an app's state is cached (in v2 of @aragon/api). This means on the next load, only new events will get loaded. We'll likely need to use a caching mechanism for the forwarded actions too, such that we load the initial scan state from cache and keep writing to it on each new forwarded action.
    • It would be great to prune forwarded actions at some point (e.g. on completion, the target app can choose to dismiss it and remove it from its list of forwarded actions)
    • What happens if the forwarding path involves multiple forwarders? Right now, each forwarder (assuming they implement this API) would create a new entry in the forwarding actions, and the target app would likely be confused with the entries since there's nothing linking them together.
    • I am still thinking about the WebWorker script as being "committed" state, such that it should very closely mirror contract state. As such, my hunch is that the subscription to the forwarded actions should be a frontend-only use, likely masked in @aragon/api-react.
  • Metadata
    • I'm interested in exactly how you envision this being used. Is it to share some information between apps when a new event comes in (e.g. Projects to DotVoting)?
@@ -361,6 +361,65 @@ App contexts can be used to display specific views in your app or anything else

Returns **[Observable](https://rxjs-dev.firebaseapp.com/api/index/class/Observable)**: An multi-emission observable that emits app contexts as they are received.

### newForwarededAction

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member
Suggested change
### newForwarededAction
### newForwardedAction

Can we also move these to be above the context docs?

I think we may also shift some things around (now that we have the freedom with @aragon/api@2) to namespace some things, like identity.requestModification() and forwarding.newAction(), but we can do that after.


Initialize a forwarded action in the registry with state = 0.

This function creates a new entry in the forwarded action registry, which is stored within the wrapper, and accessed by apps. Initialization should typically occur when the action is received by the Forwarder.

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member
Suggested change
This function creates a new entry in the forwarded action registry, which is stored within the wrapper, and accessed by apps. Initialization should typically occur when the action is received by the Forwarder.
This function creates a new entry in the forwarded action registry, which is stored within the wrapper, and accessed by apps. This should typically be called when an action is received by the Forwarder.
#### Parameters

- `actionId` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** The ID assigned to the forwarded action in the Forwarder (e.g. a vote Id)
- `evmScript` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** The raw evmScript received by the Forwarder.

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member
Suggested change
- `evmScript` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** The raw evmScript received by the Forwarder.
- `evmScript` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** The raw EVMScript received by the Forwarder.

Update the state of an existing forwarded action.

This function accepts an existing `actionId` to look up the existing action, then updates its `state` and, if updated, the `evmScript`.

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member
Suggested change
This function accepts an existing `actionId` to look up the existing action, then updates its `state` and, if updated, the `evmScript`.
This function accepts an existing `actionId` to look up the existing action, then updates its `state` and, if included, also the `evmScript`.

- `actionId` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** The ID assigned to the forwarded action in the Forwarder (e.g. a vote Id)
- `state` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** The state of the current Forwarder. 0: pending; 1: executed; 2: rejected
- `evmScript` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** The raw evmScript modified by the Forwarder. (optional, default `''`)

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member
Suggested change
- `evmScript` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** The raw evmScript modified by the Forwarder. (optional, default `''`)
- `evmScript` **[string](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String)** (optional): The raw evmScript modified by the Forwarder

Can we leave this without a default (i.e. undefined)?

// that only contains actions targeting the proxy
map(actions => ({
event: 'ForwardedActions',
returnValues: getProxyActions(actions)

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member

I wouldn't do this transformation here. If you need it inside the script (not sure if I would recommend it; perhaps this could be a frontend only thing? I still need to think a bit on this), we can add it to @aragon/api like we do with accounts.

return actions
}
},
[] // actions seed

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member

Rather than using an array here... can we just use a key value mapping (e.g. Map or an object)?

Especially since the BehaviourSubject is initialized with an object.

)
if (updateIndex === -1) {
// set the last address as the target of the forwarded action
const target = evmScript ? this.decodeTransactionPath(evmScript).pop().to : ''

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member

Not 100% sure this works as intended yet; I think decodeTransactionPath() only does one layer of EVMScript "unwrapping" (we will soon be adding the ability to decode multiple layers).

Right now, if you have a flow like Token Manager -> Voting -> Finance, I think this would only get you the Voting app.

if (updateIndex === -1) {
// set the last address as the target of the forwarded action
const target = evmScript ? this.decodeTransactionPath(evmScript).pop().to : ''
currentApp && actions.push({ currentApp, actionId, target, evmScript, state })

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member

Let's use an if here instead

* Initialize the appMetadata observable
*/
initAppMetadata () {
this.appMetadata = new BehaviorSubject({}).pipe(

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 3, 2019

Member

Should this also do a scan, to keep all the metadata published in memory?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.