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

Refactor API: ForwardingPath descriptions + App intent actions #256

Merged
merged 85 commits into from
Dec 30, 2020

Conversation

0xGabi
Copy link
Contributor

@0xGabi 0xGabi commented Aug 31, 2020

This PR includes most of the changes discussed in #132

One of the most relevant changes from the spec is that we don't have an Intent object as it was an intermediary step that ends up adding more complexity to the code. Instead of giving an intent we calculate the path and then create a ForwardingPath object that provide extra functionality on top of the data:

  • Sign transactions with a signer
  • Describe the path using Radspec
  • Apply pre-transactions to the path

@0xGabi 0xGabi changed the base branch from update-types to master August 31, 2020 21:19
@0xGabi 0xGabi changed the title Refactor API: ForwardingPath descriptions + App intent actions [WIP] Refactor API: ForwardingPath descriptions + App intent actions Aug 31, 2020
@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #256 (1731eba) into master (0c7d106) will decrease coverage by 1.06%.
The diff coverage is 0.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
- Coverage   19.89%   18.83%   -1.07%     
==========================================
  Files          74       79       +5     
  Lines        1588     1731     +143     
  Branches      337      365      +28     
==========================================
+ Hits          316      326      +10     
- Misses       1272     1405     +133     
Flag Coverage Δ
unittests 18.83% <0.27%> (-1.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/connect-core/src/entities/App.ts 0.00% <0.00%> (ø)
...ckages/connect-core/src/entities/ForwardingPath.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Organization.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Permission.ts 0.00% <ø> (ø)
packages/connect-core/src/entities/Repo.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Role.ts 0.00% <0.00%> (ø)
packages/connect-core/src/entities/Transaction.ts 0.00% <0.00%> (ø)
packages/connect-core/src/index.ts 0.00% <0.00%> (ø)
packages/connect-core/src/utils/abi.ts 0.00% <0.00%> (ø)
packages/connect-core/src/utils/abis.ts 0.00% <0.00%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c7d106...1731eba. Read the comment docs.

@@ -117,4 +99,57 @@ export default class App {
organization: null,
}
}

contract(): Contract {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added two helpers to facilitate the interaction using Ethers Contract and Interface for the app.

readonly destination: AppOrAddress
readonly transactions: Transaction[]

constructor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I finally decided to construct a ForwardingPath with the list of Transactions and the destination. This is relevant as the description of a forwarding path happened as a second step when we call describe() method.

let description: StepDescribed[] = []
if (this.transactions.length > 0) {
try {
description = await describePath(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We first describe the current list of Transactions getting a list of DescribedSteps that include a description string and the annotated description. We use the DescribedStep object to construct the ForwardingPathDescription class that provides further data transformations (toString, tree, reduce).

facuspagnuolo and others added 12 commits September 10, 2020 15:13
* agreement: handle null signers

* agreement: avoid single line if statement

Co-authored-by: Pierre Bertet <[email protected]>

Co-authored-by: Pierre Bertet <[email protected]>
* agreement: update staking ABI

* agreement: fix test
* core: fix pre approvals

* DisputableVoting: Fix pre approvals
* DisputableVoting: Implement voting checks

* DisputableVoting: Make ethers provider dep explicit

* DisputableVoting: Fix tests

Co-authored-by: 0xGabi <[email protected]>
packages/connect-core/src/entities/App.ts Outdated Show resolved Hide resolved
}

get appName(): string {
return this.artifact.appName
async roles(): Promise<Role[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a onRoles() while we are at it?

packages/connect-core/src/entities/App.ts Outdated Show resolved Hide resolved
packages/connect-core/src/entities/App.ts Outdated Show resolved Hide resolved
packages/connect-thegraph/src/parsers/repos.ts Outdated Show resolved Hide resolved
packages/connect-core/src/types.ts Outdated Show resolved Hide resolved
packages/connect-core/src/transactions/TransactionPath.ts Outdated Show resolved Hide resolved
packages/connect-core/src/entities/App.ts Outdated Show resolved Hide resolved
@@ -113,7 +133,16 @@ export default class Organization {
)
}

async acl(): Promise<App> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it’s super nice yes! 👍

packages/connect-core/src/entities/App.ts Outdated Show resolved Hide resolved
@0xGabi 0xGabi merged commit 4475d15 into master Dec 30, 2020
@0xGabi 0xGabi deleted the refactor-api branch December 30, 2020 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants