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.

Sign up for GitHub

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

Bug/multisig nonce selection #1516

Merged
merged 18 commits into from
Apr 16, 2024
Merged

Bug/multisig nonce selection #1516

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d259700
Remove a requestWithRetries
adamgall Apr 4, 2024
932fc1b
Add new property to SafeInfoResponseWithGuard
adamgall Apr 4, 2024
b9171bb
Use `nonceWithPending` on the ProposalCreatePage
adamgall Apr 4, 2024
cd8c7ce
Yeah it doesn't seem like we need that check
adamgall Apr 4, 2024
24c4401
No more `nonceWithPending`
adamgall Apr 4, 2024
4698383
Found one more place in the app where we're creating a transaction bu…
adamgall Apr 4, 2024
b1f5800
Oh look there's already a function on SafeAPI to do this
adamgall Apr 4, 2024
2e252ab
whitespace
adamgall Apr 4, 2024
6fe9a41
variable naming
adamgall Apr 4, 2024
19e9ae4
syntax cleanup
adamgall Apr 4, 2024
c37ac43
Merge branch 'develop' into bug/multisig-nonce-selection
adamgall Apr 6, 2024
697a310
Merge branch 'develop' into bug/multisig-nonce-selection
adamgall Apr 8, 2024
43253a1
Merge commit '7d72e7d5f8fb7a3575f58650c713243d84856e18' into bug/mult…
adamgall Apr 10, 2024
95f769b
Create helper in useSafeAPI to get Safe Info + Next Nonce
adamgall Apr 10, 2024
418892d
Use that helper in all the places
adamgall Apr 10, 2024
b5247fc
When loading New Proposal page, re-fetch Safe Info
adamgall Apr 10, 2024
773bb5e
Allow Formik to reinitialize it's initial values, to account for pote…
adamgall Apr 10, 2024
6e9a076
Merge branch 'develop' into bug/multisig-nonce-selection
adamgall Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 5 additions & 12 deletions src/hooks/DAO/loaders/useFractalNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { NodeAction } from '../../../providers/App/node/action';
import { useNetworkConfig } from '../../../providers/NetworkConfig/NetworkConfigProvider';
import { Node } from '../../../types';
import { mapChildNodes } from '../../../utils/hierarchy';
import { useAsyncRetry } from '../../utils/useAsyncRetry';
import { useLazyDAOName } from '../useDAOName';
import { useFractalModules } from './useFractalModules';

Expand All @@ -33,7 +32,6 @@ export const useFractalNode = (
const { getDaoName } = useLazyDAOName();

const lookupModules = useFractalModules();
const { requestWithRetries } = useAsyncRetry();

const formatDAOQuery = useCallback((result: { data?: DAOQueryQuery }, _daoAddress: string) => {
if (!result.data) {
Expand Down Expand Up @@ -103,20 +101,15 @@ export const useFractalNode = (
try {
if (!safeAPI) throw new Error('SafeAPI not set');

safeInfo = await requestWithRetries(
() => safeAPI.getSafeInfo(utils.getAddress(_daoAddress)),
5,
);
const address = utils.getAddress(_daoAddress);
const safeInfoResponse = await safeAPI.getSafeInfo(address);
const nextNonce = await safeAPI.getNextNonce(address);
safeInfo = { ...safeInfoResponse, nonce: nextNonce };
Copy link
Member Author

Choose a reason for hiding this comment

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

Look at that, there's a function right on the Safe API for getting the next nonce.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lool I'm glad you didn't discover this before having to implement it yourself, which led to me understanding a wee bit more what's going on behind the scenes

} catch (e) {
reset({ error: true });
return;
}

if (!safeInfo) {
reset({ error: true });
return;
}

Comment on lines -113 to -117
Copy link
Member Author

Choose a reason for hiding this comment

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

This shouldn't ever be null/undefined, the catch and return will handle any Safe API errors

// if here, we have a valid Safe!

action.dispatch({
Expand All @@ -129,7 +122,7 @@ export const useFractalNode = (
payload: safeInfo,
});
},
[action, lookupModules, requestWithRetries, reset, safeAPI],
[action, lookupModules, reset, safeAPI],
);

useEffect(() => {
Expand Down
14 changes: 8 additions & 6 deletions src/hooks/DAO/loaders/useLoadDAONode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,16 @@ export const useLoadDAONode = () => {
logError('graphQL query failed');
return { error: 'errorFailedSearch' };
}
const safe = await safeAPI.getSafeInfo(_daoAddress);
const fractalModules = await lookupModules(safe.modules);
const daoName = await getDaoName(utils.getAddress(safe.address), graphNodeInfo.daoName);

const sanitizedDaoAddress = utils.getAddress(_daoAddress);
const safeInfo = await safeAPI.getSafeInfo(sanitizedDaoAddress);
const nextNonce = await safeAPI.getNextNonce(sanitizedDaoAddress);
const safeInfoWithGuard = { ...safeInfo, nonce: nextNonce };

const node: FractalNode = Object.assign(graphNodeInfo, {
daoName,
safe,
fractalModules,
daoName: await getDaoName(sanitizedDaoAddress, graphNodeInfo.daoName),
safe: safeInfoWithGuard,
fractalModules: await lookupModules(safeInfo.modules),
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a place where we're loading up Safe data and not getting it from / putting it into global state, so want to make sure to put the proper next nonce on this one.

});

// TODO we could cache node here, but should be careful not to cache
Expand Down
8 changes: 6 additions & 2 deletions src/hooks/DAO/useClawBack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ export default function useClawBack({ childSafeInfo, parentAddress }: IUseClawBa
const childSafeBalance = await safeAPI.getBalances(
utils.getAddress(childSafeInfo.daoAddress),
);
const parentSafeInfo = await safeAPI.getSafeInfo(utils.getAddress(parentAddress));

const santitizedParentAddress = utils.getAddress(parentAddress);
const parentSafeInfo = await safeAPI.getSafeInfo(santitizedParentAddress);
const parentSafeNextNonce = await safeAPI.getNextNonce(santitizedParentAddress);

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a place where we're loading up Safe data and not getting it from / putting it into global state, so want to make sure to put the proper next nonce on this one.

if (canUserCreateProposal && parentAddress && childSafeInfo && parentSafeInfo) {
const abiCoder = new ethers.utils.AbiCoder();
const fractalModule = childSafeInfo.fractalModules!.find(
Expand Down Expand Up @@ -88,7 +92,7 @@ export default function useClawBack({ childSafeInfo, parentAddress }: IUseClawBa
values: transactions.map(tx => tx.value),
calldatas: transactions.map(tx => tx.calldata),
},
nonce: parentSafeInfo.nonce,
nonce: parentSafeNextNonce,
pendingToastMessage: t('clawBackPendingToastMessage'),
failedToastMessage: t('clawBackFailedToastMessage'),
successToastMessage: t('clawBackSuccessToastMessage'),
Expand Down