Bug/multisig nonce selection #1516

adamgall · 2024-04-04T14:17:14Z

Outstanding questions

Are there any other places where a multisig proposal is prepared, other than the obvious "new proposal" page which has been handled in this PR?
A dev comment

Testing

Using a multisg Safe
Queue up a transaction so it's pending, but do not execute it! Let's assume this queued transaction has a Safe nonce of "5".
Then begin to create a new Proposal/Transaction in Fractal
See how the "nonce" field for this proposal you're beginning to create is "6"! Before this PR, it also would have been "5" which would cause a conflict on the Safe.

called `nonceWithPending`, which will be the next nonce to use which takes into account all "pending" transactions on the safe

netlify · 2024-04-04T14:17:30Z

✅ Deploy Preview for fractal-dev ready!

Name	Link
🔨 Latest commit	`6e9a076`
🔍 Latest deploy log	https://app.netlify.com/sites/fractal-dev/deploys/661e8a0ed232f0000876c6d0
😎 Deploy Preview	https://deploy-preview-1516.app.dev.fractalframework.xyz
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/hooks/DAO/loaders/useFractalNode.ts

DarksightKellar

LGTM

Knowledge gap question: How does nonceWithPending fix this bug exactly? I understand as much as it being equal to the tx count on the DAO, so as to somehow eagerly set the nonce of the next tx to that count (+1?). Just not sure sure how/where the flow makes that happen. We can chat about it later (or if it can be easily explained in a comment, I'll take ti)

DarksightKellar · 2024-04-04T15:27:22Z

To consider regarding "Are there any other places where a multisig proposal is prepared, other than the obvious "new proposal" page which has been handled in this PR?":

Updating DAO settings still seems to keep the same nonce. But, David mentioned something about submitting a proposal of the same nonce as a currently pending proposal that one might want to invalidate. That works currently, but any other DAO setting update (after an unrelated setting update) is still stuck on the now used up nonce. Probably should separate the two intentions (or cook up a way to reject a proposal without having to manually overwrite it, which sounds like a much bigger lift).

What I tried:

raised edit DAO name proposal. Not executed. Nonce was 2.
raised 2nd edit DAO name proposal. Same nonce with manual change (this is the bug/feature)
executed 2nd edit proposal. 1st proposal now shows as "rejected".
attempted to add a signer to the DAO. Failed because nonce was auto-set to 2.

adamgall · 2024-04-04T16:23:08Z

LGTM

Knowledge gap question: How does nonceWithPending fix this bug exactly? I understand as much as it being equal to the tx count on the DAO, so as to somehow eagerly set the nonce of the next tx to that count (+1?). Just not sure sure how/where the flow makes that happen. We can chat about it later (or if it can be easily explained in a comment, I'll take ti)

Sure, I'll try to explain here.

Context

Safe contracts utilize an internal nonce counter. Every transaction that is executed on a Safe must have a nonce which is equal to the last transaction's nonce + 1. The transaction is invalid and will fail, otherwise.

Next: the way that Multisig Safes work, is that transactions can be "pending". That means that they haven't been submitted onchain yet, they only exist in the web2 world, and in this API based web2 world all of the necessary signers create their signatures for this potential transaction and they're all aggregated in an API. The data that they're signing (the pending transaction) is all of the data that makes up a Safe transaction, including that nonce.

We utilize the Safe API to get information about a Safe, including its prior transactions. There are a couple of endpoints that we hit. One of those endpoints tells us the nonce that the next transaction must be. We were currently only using the data from this endpoint. Another endpoint (which we weren't using before), gives us a list of all transactions, including any pending ones. the new nonceWithPending property I created is set to the number of transactions in this response. That way, the next nonce being used in a Safe transaction that Fractal creates and will send to the Safe API, has a nonce which is one more than however many pending transactions there are.

... aaaand typing this out, I realize there will be a bug if a Safe already has two pending transactions with the same nonce (these are conflicting, as only one of them will be able to be broadcast onchain, but it's a totally valid state for the UI).

adamgall · 2024-04-04T16:25:43Z

To consider regarding "Are there any other places where a multisig proposal is prepared, other than the obvious "new proposal" page which has been handled in this PR?":

Updating DAO settings still seems to keep the same nonce. But, David mentioned something about submitting a proposal of the same nonce as a currently pending proposal that one might want to invalidate. That works currently, but any other DAO setting update (after an unrelated setting update) is still stuck on the now used up nonce. Probably should separate the two intentions (or cook up a way to reject a proposal without having to manually overwrite it, which sounds like a much bigger lift).

What I tried:

raised edit DAO name proposal. Not executed. Nonce was 2.

raised 2nd edit DAO name proposal. Same nonce with manual change (this is the bug/feature)

executed 2nd edit proposal. 1st proposal now shows as "rejected".

attempted to add a signer to the DAO. Failed because nonce was auto-set to 2.

Awesome, great call out. I'll go investigate this section of the app.

edit: @DarksightKellar if you don't mind trying this again, i hope it's fixed now

adamgall · 2024-04-04T16:45:30Z

Maybe a better question for the group (@tomstuart123 @Da-Colon @mudrila) is: are there any situations in which we don't want to take into account pending multisig transactions, when crafting new ones?

I'm going to assume no, and move forward with that.

Operating under the assumption that any time in the app we're utilizing the `nonce`, it's because we want to create a proposal. Under this assumption, we'll always want to take "pending transactions" into account. Also, this new code handles the cases in which: - There are no pending transactions - There are multiple pending transactions with the same nonce

…t _not_ using the Safe data from global state

adamgall · 2024-04-04T17:57:00Z

src/hooks/DAO/loaders/useFractalNode.ts

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


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

adamgall · 2024-04-04T17:57:21Z

src/hooks/DAO/loaders/useFractalNode.ts

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


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

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

adamgall · 2024-04-04T18:02:40Z

src/hooks/DAO/loaders/useLoadDAONode.ts

+
+          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),


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.

adamgall · 2024-04-04T18:03:32Z

src/hooks/DAO/useClawBack.ts

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


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.

tomstuart123 · 2024-04-05T13:26:50Z

Hey @adamgall , thanks for this

So tested a few proposals with this at this DAO - https://deploy-preview-1516.app.dev.fractalframework.xyz/home?dao=sep:0x463e491e72f20ba1bD70EF79d0D7a777Fd762Fa4

What worked:

Submitting an unexecuted nonce at 4, led to nonce at 5 auto updating for all areas of the app I could think of (i.e. i] send assets ii] create proposal iii] create template iv] all settings changes v] add subsafe)

What didn't work:

[note this is expected behaviour] if you try and create any of the proposals above BEFORE, the UI has read that the proposal is in the list, the old nonce will be used. As expected and no change required
[to be debated @adamgall ] - if you add multiple proposals in the queue to be executed, the nonce doesn't continue increasing. It will only ever increase by max 1. e.g. in the below 'send token = nonce 4', 'update safe name = nonce 5' and 'update snapshot space = nonce 5'. SImilarly if I try and now create a new proposal across the app, the auto populate is nonce 5

The benefits of this implementation - I guess this stops increasing the nonce for mult-proposals that are at the same nonce i.e. will be wiped out.
The cons of this implementation - If you're creating multi-proposals that go straight to pending, you see the nonce auto increase on the first... but then after that the nonce will stay flat

@adamgall can you confirm if this is intended behaviour

adamgall · 2024-04-05T13:31:50Z

@tomstuart123 the "debatable" behavior you described is not how I intended it to work. looking at your Safe through the Safe's UI tells me that any new proposals being created in Fractal should have a nonce 6.

Now, I did explicitly test this, and it was working for me as I just described here. So, not sure what's going on. Care to hop on a call and figure this out?

Not ready for merge yet, more work to do.

adamgall · 2024-04-06T00:08:16Z

After speaking with @tomstuart123 , there's some more work to do on this branch.

The outstanding issue is that we only fetch the "next nonce" once, when a Safe is initially loaded in Fractal. If a transaction is created in Fractal (and not executed), then another transaction is created, that second one will still be using the "next nonce" from the first one.

We need to figure out the cleanest way to re-fetch the "next nonce"...

on an interval? blah
whenever a proposal is being created? maybe...
any other ideas?

Da-Colon · 2024-04-09T16:27:30Z

After speaking with @tomstuart123 , there's some more work to do on this branch.

The outstanding issue is that we only fetch the "next nonce" once, when a Safe is initially loaded in Fractal. If a transaction is created in Fractal (and not executed), then another transaction is created, that second one will still be using the "next nonce" from the first one.

We need to figure out the cleanest way to re-fetch the "next nonce"...

on an interval? blah

whenever a proposal is being created? maybe...

any other ideas?

The only other thing that comes to mind, is way more tedious but check nonce as things happen.

Loading the Form, check the nonce
Clicking Submit, Check the nonce and show an error if nonce has changed or needs to be updated.

…isig-nonce-selection

adamgall · 2024-04-10T19:38:10Z

@tomstuart123 @Da-Colon @mudrila @DarksightKellar

Updated the code so that whenever pulling up the New Proposal page, we re-fetch the Safe Data to make sure the most recent "next nonce" is being used.

Still seeing some delay of about a minute after a new proposal is queued up before the nonce updates, but I think that's baked into the caching of our Safe Service client code.

This is ready for re-review.

…ntially updated nonce

Da-Colon

read through code, saw it live, Ship it!

Da-Colon · 2024-04-10T23:11:21Z

Additional question. This covers "New Proposal", What about the other spots such as 'Settings' and 'Edit' and Treasury Pages (for Send Assets)?

adamgall · 2024-04-10T23:15:25Z

Additional question. This covers "New Proposal", What about the other spots such as 'Settings' and 'Edit' and Treasury Pages (for Send Assets)?

dammit

DarksightKellar · 2024-04-11T15:21:28Z

Some observations:

When there are multiple pending proposals, they must be executed in order of their nonces. Attempting to execute a nonce-7 proposal while a nonce-6 is pending fails with a non-descriptive error. If you go execute nonce-6, then nonce-7, that works fine. Tested with up to 4 pending proposals, and the behaviour is easily reproducible. This is likely expected behaviour though.
I ran into an edge case where I created a proposal with nonce 7, and before it was visible on the UI, attempted to create another. The nonce assigned to it was 7. I went back, did a couple refreshes as I impatiently waited for the just-created proposal to show up. When it did I tried again, and still got nonce 7. (in the time it took me to think and type this out, without refreshing, the nonce is now 8, so, a timing thing, it appears. probably not worth digging into)
EDIT: Sorry, I missed this part of your comment:

Still seeing some delay of about a minute after a new proposal is queued up before the nonce updates

I just used the 8th nonce to edit the dao name. waited for it to show up in proposal list, created new proposal, which was assigned nonce 9. DID NOT actually hit "create", went back to edit dao name (with previous edit still unexecuted). Also nonce 9.

For the most part these feel like edge case that won't matter in real world use.

adamgall · 2024-04-11T19:03:51Z

Good notes, thanks @DarksightKellar

Some observations:

When there are multiple pending proposals, they must be executed in order of their nonces. Attempting to execute a nonce-7 proposal while a nonce-6 is pending fails with a non-descriptive error. If you go execute nonce-6, then nonce-7, that works fine. Tested with up to 4 pending proposals, and the behaviour is easily reproducible. This is likely expected behaviour though.

Yep, this is a bug (nice find), but out of scope for this PR. I've opened an issue: #1536

I ran into an edge case where I created a proposal with nonce 7, and before it was visible on the UI, attempted to create another. The nonce assigned to it was 7. I went back, did a couple refreshes as I impatiently waited for the just-created proposal to show up. When it did I tried again, and still got nonce 7. (in the time it took me to think and type this out, without refreshing, the nonce is now 8, so, a timing thing, it appears. probably not worth digging into)
EDIT: Sorry, I missed this part of your comment:

Still seeing some delay of about a minute after a new proposal is queued up before the nonce updates

I just used the 8th nonce to edit the dao name. waited for it to show up in proposal list, created new proposal, which was assigned nonce 9. DID NOT actually hit "create", went back to edit dao name (with previous edit still unexecuted). Also nonce 9.

For the most part these feel like edge case that won't matter in real world use.

These last two points, yeah, not ideal, but not sure how much more I can do about them.

adamgall · 2024-04-12T15:34:26Z

To Do:

Make sure Nonce is updated on Send Tokens proposal creation, too.

tomstuart123

Agreed with @adamgall that we make the 'auto nonce update without refresh' in other areas of the app in another issue

see here

This PR is approved

https://github.com/orgs/decentdao/projects/32/views/3?pane=issue&itemId=59938098

adamgall added 3 commits April 4, 2024 09:59

Remove a requestWithRetries

d259700

Add new property to SafeInfoResponseWithGuard

932fc1b

called `nonceWithPending`, which will be the next nonce to use which takes into account all "pending" transactions on the safe

Use nonceWithPending on the ProposalCreatePage

b9171bb

adamgall self-assigned this Apr 4, 2024

adamgall requested a review from mudrila April 4, 2024 14:17

adamgall requested review from DarksightKellar, tomstuart123 and Da-Colon April 4, 2024 14:17

adamgall commented Apr 4, 2024

View reviewed changes

src/hooks/DAO/loaders/useFractalNode.ts Outdated Show resolved Hide resolved

DarksightKellar previously approved these changes Apr 4, 2024

View reviewed changes

Yeah it doesn't seem like we need that check

cd8c7ce

adamgall added 6 commits April 4, 2024 13:41

Found one more place in the app where we're creating a transaction bu…

4698383

…t _not_ using the Safe data from global state

Oh look there's already a function on SafeAPI to do this

b1f5800

whitespace

2e252ab

variable naming

6fe9a41

syntax cleanup

19e9ae4

adamgall commented Apr 4, 2024

View reviewed changes

mudrila previously approved these changes Apr 5, 2024

View reviewed changes

Da-Colon previously approved these changes Apr 5, 2024

View reviewed changes

adamgall added 2 commits April 5, 2024 20:09

Merge branch 'develop' into bug/multisig-nonce-selection

c37ac43

Merge branch 'develop' into bug/multisig-nonce-selection

697a310

Merge commit '7d72e7d5f8fb7a3575f58650c713243d84856e18' into bug/mult…

43253a1

…isig-nonce-selection

adamgall force-pushed the bug/multisig-nonce-selection branch from 92a7b8a to 43253a1 Compare April 10, 2024 20:24

adamgall added 4 commits April 10, 2024 16:25

Create helper in useSafeAPI to get Safe Info + Next Nonce

95f769b

Use that helper in all the places

418892d

When loading New Proposal page, re-fetch Safe Info

b5247fc

Allow Formik to reinitialize it's initial values, to account for pote…

773bb5e

…ntially updated nonce

Da-Colon approved these changes Apr 10, 2024

View reviewed changes

adamgall mentioned this pull request Apr 11, 2024

Execution order of multiple pending multisig transactions #1536

Closed

adamgall mentioned this pull request Apr 16, 2024

Make sure correct nonce is being used on Send Assets proposal creation modal #1568

Closed

tomstuart123 approved these changes Apr 16, 2024

View reviewed changes

Merge branch 'develop' into bug/multisig-nonce-selection

6e9a076

adamgall merged commit 5d64773 into develop Apr 16, 2024
7 checks passed

adamgall deleted the bug/multisig-nonce-selection branch April 16, 2024 14:26

adamgall mentioned this pull request Apr 17, 2024

2024-04-17 release #1574

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/multisig nonce selection #1516

Bug/multisig nonce selection #1516

adamgall commented Apr 4, 2024 •

edited

Loading

netlify bot commented Apr 4, 2024 •

edited

Loading

DarksightKellar left a comment

DarksightKellar commented Apr 4, 2024

adamgall commented Apr 4, 2024

adamgall commented Apr 4, 2024 •

edited

Loading

adamgall commented Apr 4, 2024 •

edited

Loading

adamgall Apr 4, 2024

adamgall Apr 4, 2024

DarksightKellar Apr 5, 2024

adamgall Apr 4, 2024

adamgall Apr 4, 2024

tomstuart123 commented Apr 5, 2024

adamgall commented Apr 5, 2024

adamgall commented Apr 6, 2024

Da-Colon commented Apr 9, 2024

adamgall commented Apr 10, 2024 •

edited

Loading

Da-Colon left a comment

Da-Colon commented Apr 10, 2024

adamgall commented Apr 10, 2024

DarksightKellar commented Apr 11, 2024 •

edited

Loading

adamgall commented Apr 11, 2024 •

edited

Loading

adamgall commented Apr 12, 2024

tomstuart123 left a comment

Bug/multisig nonce selection #1516

Bug/multisig nonce selection #1516

Conversation

adamgall commented Apr 4, 2024 • edited Loading

Outstanding questions

Testing

netlify bot commented Apr 4, 2024 • edited Loading

✅ Deploy Preview for fractal-dev ready!

DarksightKellar left a comment

Choose a reason for hiding this comment

DarksightKellar commented Apr 4, 2024

adamgall commented Apr 4, 2024

Context

adamgall commented Apr 4, 2024 • edited Loading

adamgall commented Apr 4, 2024 • edited Loading

adamgall Apr 4, 2024

Choose a reason for hiding this comment

adamgall Apr 4, 2024

Choose a reason for hiding this comment

DarksightKellar Apr 5, 2024

Choose a reason for hiding this comment

adamgall Apr 4, 2024

Choose a reason for hiding this comment

adamgall Apr 4, 2024

Choose a reason for hiding this comment

tomstuart123 commented Apr 5, 2024

adamgall commented Apr 5, 2024

adamgall commented Apr 6, 2024

Da-Colon commented Apr 9, 2024

adamgall commented Apr 10, 2024 • edited Loading

Da-Colon left a comment

Choose a reason for hiding this comment

Da-Colon commented Apr 10, 2024

adamgall commented Apr 10, 2024

DarksightKellar commented Apr 11, 2024 • edited Loading

adamgall commented Apr 11, 2024 • edited Loading

adamgall commented Apr 12, 2024

tomstuart123 left a comment

Choose a reason for hiding this comment

adamgall commented Apr 4, 2024 •

edited

Loading

netlify bot commented Apr 4, 2024 •

edited

Loading

adamgall commented Apr 4, 2024 •

edited

Loading

adamgall commented Apr 4, 2024 •

edited

Loading

adamgall commented Apr 10, 2024 •

edited

Loading

DarksightKellar commented Apr 11, 2024 •

edited

Loading

adamgall commented Apr 11, 2024 •

edited

Loading