Refactor: Add Intent entity + refactor calculate path logic #225
Conversation
| @@ -90,11 +90,11 @@ export default class App { | |||
| return this.artifact.abi | |||
| } | |||
|
|
|||
| get intents(): AppIntent[] { | |||
| get methods(): AppMethod[] { | |||
There was a problem hiding this comment.
I decide to change the name to methods instead of intents as it was confusing. I'm also wondering if we may want to remove those methods at all 🤔
| @@ -61,6 +61,19 @@ export default class App { | |||
| return this.organization.connection.orgConnector | |||
| } | |||
|
|
|||
| contract(): ethers.Contract { | |||
There was a problem hiding this comment.
I include a helper to have access to an Ethers contract object to make it super easy to interact with the blockchain for the particular App.
| } | ||
|
|
||
| // Retrieve a single forwarding path. Defaults to the shortest one. | ||
| async path({ actAs, path }: PathOptions): Promise<TransactionPath> { |
There was a problem hiding this comment.
We removed all the logic related to the description of a path and handle that responsibility to the Forwarding Path.
|
|
||
| // Retrieve the different possible forwarding paths. | ||
| async paths({ actAs, path }: PathOptions): Promise<TransactionPath[]> { | ||
| // TODO: support logic to calculate multiple Forwarding paths |
There was a problem hiding this comment.
We left the calculation of multiple Forwarding Path for a later PR.
| } | ||
|
|
||
| const createForwarderTransaction = createForwarderTransactionBuilder( | ||
| sender, | ||
| directTransaction | ||
| ) | ||
|
|
||
| const buildForwardingPath = async ( |
There was a problem hiding this comment.
Extracted this logic as it was used multiple times.
| @@ -181,34 +150,26 @@ async function calculateForwardingPath( | |||
| */ | |||
| export async function calculateTransactionPath( | |||
| sender: string, | |||
| destination: string, | |||
| destinationApp: App, | |||
There was a problem hiding this comment.
We now accept an App entity instead.
| } | ||
|
|
||
| // Make sure the method signature is correct | ||
| const method = validateMethod(destination, methodSignature, destinationApp) |
There was a problem hiding this comment.
We no longer need this logic because we use the Ethers interface object inside createDirectTransactionForApp: https://github.com/aragon/connect/pull/224/files#diff-a9188376c85e9975a721384535e75e5aR19
| ? ethers.utils.isAddress(finalForwarder) | ||
| : false | ||
|
|
||
| const method = getAppMethod(destinationApp, methodSignature) |
There was a problem hiding this comment.
We are only using the method to know if it's protected by a role. I wonder if we can get this information in other way 🤔
| @@ -249,11 +210,13 @@ export async function calculateTransactionPath( | |||
| (role) => role.name === method.roles[0] | |||
| ) | |||
| const allowedEntities = | |||
| role?.permissions?.map((permission) => permission.granteeAddress) || [] | |||
| role?.permissions | |||
| ?.filter((permission) => permission.allowed) | |||
There was a problem hiding this comment.
We missed to check if the permission was allowed.
| // No forwarders can perform the requested action | ||
| if (forwardersWithPermission.length === 0) { | ||
| return { path: [] } | ||
| return [] |
There was a problem hiding this comment.
We return an array the Transaction object.
0xGabi
force-pushed
the
refactor-intent
branch
from
cc8cd91
to
e49c607
Compare
Codecov Report
@@ Coverage Diff @@
## refactor-transaction #225 +/- ##
=====================================================
Coverage 35.96% 35.96%
=====================================================
Files 97 97
Lines 1760 1760
Branches 277 277
=====================================================
Hits 633 633
Misses 1127 1127
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Closes #217
Builds on #224