-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: implement arbitrary transactions via reputation #3933
Conversation
If this PR is not ready for review, can you move it back to draft please? |
networkClient: colonyClient.networkClient, | ||
parentDomainNativeId: rootDomain.nativeId, | ||
parentDomainSkillId: rootDomain.nativeSkillId, | ||
domainNativeId: rootDomain.nativeId, | ||
domainSkillId: rootDomain.nativeSkillId, | ||
}); | ||
const { skillId } = yield call( | ||
[colonyClient, colonyClient.getDomain], | ||
Id.RootDomain, | ||
); | ||
|
||
const { key, value, branchMask, siblings } = yield call( | ||
colonyClient.getReputation, | ||
skillId, | ||
AddressZero, | ||
); | ||
|
||
return { | ||
context: ClientType.VotingReputationClient, | ||
methodName: 'createMotion', | ||
identifier: colonyAddress, | ||
params: [ | ||
Id.RootDomain, | ||
childSkillIndex, | ||
AddressZero, | ||
encodedAction, | ||
key, | ||
value, | ||
branchMask, | ||
siblings, | ||
], | ||
group: { | ||
key: batchKey, | ||
id: metaId, | ||
index: 0, | ||
}, | ||
ready: false, | ||
}; | ||
} | ||
|
||
const transactionParams = yield getCreateMotionParams(); | ||
// create transactions | ||
yield fork(createTransaction, createMotion.id, transactionParams); | ||
|
||
if (annotationMessage) { | ||
yield fork(createTransaction, annotateRootMotion.id, { | ||
context: ClientType.ColonyClient, | ||
methodName: 'annotateTransaction', | ||
identifier: colonyAddress, | ||
params: [], | ||
group: { | ||
key: batchKey, | ||
id: metaId, | ||
index: 1, | ||
}, | ||
ready: false, | ||
}); | ||
} | ||
|
||
yield takeFrom(createMotion.channel, ActionTypes.TRANSACTION_CREATED); | ||
if (annotationMessage) { | ||
yield takeFrom( | ||
annotateRootMotion.channel, | ||
ActionTypes.TRANSACTION_CREATED, | ||
); | ||
} | ||
|
||
yield initiateTransaction(createMotion.id); | ||
|
||
const { | ||
payload: { | ||
receipt: { transactionHash: txHash }, | ||
}, | ||
} = yield waitForTxResult(createMotion.channel); | ||
|
||
yield createActionMetadataInDB(txHash, customActionTitle); | ||
|
||
if (annotationMessage) { | ||
yield uploadAnnotation({ | ||
txChannel: annotateRootMotion, | ||
message: annotationMessage, | ||
txHash, | ||
}); | ||
} | ||
|
||
setTxHash?.(txHash); | ||
|
||
yield put<AllActions>({ | ||
type: ActionTypes.MOTION_ARBITRARY_TRANSACTION_SUCCESS, | ||
meta, | ||
}); | ||
} catch (caughtError) { | ||
yield putError( | ||
ActionTypes.MOTION_ARBITRARY_TRANSACTION_ERROR, | ||
caughtError, | ||
meta, | ||
); | ||
} finally { | ||
txChannel.close(); | ||
} |
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.
This part of the code almost a duplication of the Root Motion Saga; I tried to reuse the Saga but faced types issues. Any ideas on better solution are welcome :)
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.
@Nortsova I left the needed changes in my PR review comment 🙌
adb6441
to
76570c4
Compare
c9e339e
to
5819a12
Compare
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.
Impressive work on this one @Nortsova 🙌
It all works as expected 💯
Installed the Reputation
extension
Started creating some transactions for the given contract address
Could see the motion got created in the logs
The incoming funds page before the mint
operation got executed
Finalised, but realised I didn't use the right transaction for minting the tokens - actually didn't use the right address parameter for the other mint
method
And after the motion was Supported
and Finalised
, could see the incoming funds
Tried the scenario of rejecting a motion
And the incoming funds page didn't change after the motion was Rejected
and Finalised
Refactoring to use the root motion saga
@Nortsova if you want to make use of the ROOT_MOTION
saga you can try out these changes.
Save the below in a .diff
and run git apply .diff
diff --git a/src/components/v5/common/ActionSidebar/partials/forms/ArbitraryTxsForm/hooks.ts b/src/components/v5/common/ActionSidebar/partials/forms/ArbitraryTxsForm/hooks.ts
index bce215888..e5f3474c6 100644
--- a/src/components/v5/common/ActionSidebar/partials/forms/ArbitraryTxsForm/hooks.ts
+++ b/src/components/v5/common/ActionSidebar/partials/forms/ArbitraryTxsForm/hooks.ts
@@ -1,11 +1,14 @@
import { Id } from '@colony/colony-js';
+import { Interface } from 'ethers/lib/utils';
import { useMemo } from 'react';
import { useWatch } from 'react-hook-form';
import { useColonyContext } from '~context/ColonyContext/ColonyContext.ts';
import { ActionTypes } from '~redux/index.ts';
+import { RootMotionMethodNames } from '~redux/types/actions/motion.ts';
import { DecisionMethod } from '~types/actions.ts';
import { mapPayload } from '~utils/actions.ts';
+import { extractColonyRoles } from '~utils/colonyRoles.ts';
import { extractColonyDomains } from '~utils/domains.ts';
import { sanitizeHTML } from '~utils/strings.ts';
import { DECISION_METHOD_FIELD_NAME } from '~v5/common/ActionSidebar/consts.ts';
@@ -30,7 +33,7 @@ export const useCreateArbitraryTxs = (
actionType:
decisionMethod === DecisionMethod.Permissions
? ActionTypes.CREATE_ARBITRARY_TRANSACTION
- : ActionTypes.MOTION_ARBITRARY_TRANSACTION,
+ : ActionTypes.ROOT_MOTION,
validationSchema,
getFormOptions,
defaultValues: useMemo(
@@ -45,18 +48,38 @@ export const useCreateArbitraryTxs = (
const commonPayload = {
annotationMessage: safeDescription,
customActionTitle: payload.title,
- transactions: payload.transactions,
colonyAddress,
};
if (payload.decisionMethod === DecisionMethod.Reputation) {
+ const contractAddresses: string[] = [];
+ const methodsBytes: string[] = [];
+
+ payload.transactions.forEach(({ contractAddress, ...item }) => {
+ try {
+ const encodedFunction = new Interface(
+ item.jsonAbi,
+ ).encodeFunctionData(
+ item.method,
+ item.args?.map((arg) => arg.value),
+ );
+ contractAddresses.push(contractAddress);
+ methodsBytes.push(encodedFunction);
+ } catch (e) {
+ console.error(e);
+ }
+ });
+
return {
...commonPayload,
+ motionParams: [contractAddresses, methodsBytes, true],
+ operationName: RootMotionMethodNames.MakeArbitraryTransaction,
colonyDomains: extractColonyDomains(colony.domains),
+ colonyRoles: extractColonyRoles(colony.roles),
};
}
- return commonPayload;
+ return { ...commonPayload, transactions: payload.transactions };
}),
});
};
diff --git a/src/redux/types/actions/motion.ts b/src/redux/types/actions/motion.ts
index adb7f8e8d..3de7c0a85 100644
--- a/src/redux/types/actions/motion.ts
+++ b/src/redux/types/actions/motion.ts
@@ -39,6 +39,7 @@ export enum RootMotionMethodNames {
MoveFunds = 'moveFunds',
Upgrade = 'upgrade',
UnlockToken = 'unlockToken',
+ MakeArbitraryTransaction = 'makeArbitraryTransactions',
}
export interface ExpenditureFundMotionPayload extends ExpenditureFundPayload {
And this should do the trick 🙌
However, I'm okay even with using a separate saga for this, so I'll approve your PR. Nice work! 🥇
Thanks, @mmioana 🙌 Me and @jakubcolony discussed a couple of solutions, and this one as well; we decided that we don't want to overcomplicate UI logic and want to leave transformation inside Saga. But now, with new changes that @jakubcolony did in #3966, it has become more complicated to reuse ROOT_MOTION. ❤️ |
5819a12
to
71cc9f1
Compare
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.
@Nortsova it makes sense and I think it's even more clear to keep the sagas separate. Just noticed your previous comment and wanted to try it out 🙌
Awesome work, it still works as expected after the rebase
Created a Custom transactions
using Reputation
Incoming
page before finalising the staked
motion
Incoming
page after finalising the staked
motion
Created another Custom transactions
with Repuation
And the Incoming funds
page remained unchanged 👍
Really cool work on this one @Nortsova 🥇
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.
Really nice and straightforward 💯
I actually fully support this new arbitratyTx
saga, I wouldn't merge it with rootMotion
, since that one is already doing 6 different things 😅 But if this turns out super simple, we can merge them down the line.
Filled out the form nicely ✔️
Motion data is logged ✔️
Fully supported it and finalized ✔️
Incoming funds 👀
When rejecting, nothing happens 👌
Just the previous funds incoming 👀
71cc9f1
to
7db7bd4
Compare
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.
Nice job getting this working so nicely! It works exactly as expected when creating a motion in the root domain:
Screen.Recording.2025-01-09.at.16.39.33.mov
No incoming funds:
Then after finalising the funds appear:
Screen.Recording.2025-01-09.at.16.41.45.mov
Only one very minor change, the Created in
field should be readonly as it is only possible to create an arbitrary transaction in the root domain.
case ColonyActionType.MakeArbitraryTransactionsMotion: | ||
// @NOTE: Enabling expenditure-related motions above temporarily (action UI will be missing) |
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.
I'm not sure if this comment is still relevant, might be worth moving the arbitrary motion above the expenditure ones just in case.
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.
Smashing work @Nortsova! Apart from the Created in field mentioned by Sam, everything looks & works great 💯
src/redux/types/actions/motion.ts
Outdated
{ | ||
customActionTitle: string; | ||
colonyAddress: Address; | ||
colonyName?: string; |
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 unused
Thanks guys! Fixed 🙌 |
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.
Great, thanks for the changes!
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.
Reapproving after rebase, let's go 🚢
Description
This issue covers the reputation (motions) flow of creating arbitrary transaction.
New saga needs to be added that should accept a target contract address and a list of arbitrary transactions, consisting of function name and a list of arguments. It should create a motion to call makeArbitraryTransactions on the Colony contract.
To get the value for action, it can use the contract ABI to create an ethers interface and then encode the call with encodeFunctionData (note there will be "2 layers" of encoding, one for the makeArbitraryTransactions's actions and the other for motion's action).
The existing block-ingestor motion handler might need to be modified to store additional data in the database.
The saga should be wired in with the action form.
Tip
Block ingestor PR: JoinColony/block-ingestor#304
Testing
Step 1. Update
src/components/v5/common/ActionSidebar/partials/forms/ArbitraryTxsForm/partials/AddTransactionModal/useGenerateABI.ts
line 21 toreturn;
(Unfortunately, we can't get a local contract from arbiscan, so we will put it manually 🙌 )Step 2. Enable Reputation Weighted extension (with "Testing governance" settings)
Step 3. "Manage colony" -> "Custom transactions"
Step 4. Create transaction with:
Contract address:
0xeF841fe1611ce41bFCf0265097EFaf50486F5111
ABI:
Step 5. Create Action with two transactions (to check that multiple transactions are working), one of transactions should be mint
Step 6. Submit action with Decision method "Reputation"
Step 7. Check that in console you can see Motion with Arbitrary transactions:
Step 8. On the finance page (http://localhost:9091/planex/incoming) check that there are no incoming funds (this means that transactions weren't called)
Step 9. Fully support motion:
Step 10. Run
npm run forward-time 1
in consoleStep 11. Refresh page and Finalize motion
Step 12. Check finance page http://localhost:9091/planex/incoming that
mint
function was called and you can see incoming funds:Step 13. Repeat Step 3 - Step 8.
Step 14. Fully reject motion:
Step 15. Verify that functions weren't called and that there is nothing new on http://localhost:9091/planex/incoming
Any other tests are welcome 🙌
Contributes to #3536