-
Notifications
You must be signed in to change notification settings - Fork 439
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: poll recovery state of Safe #2767
Conversation
Branch preview✅ Deploy successful! |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Coverage report
Show new covered files 🐣
Test suite run success1078 tests passing in 147 suites. Report generated by 🧪jest coverage report action from f70fff8 |
const queue = await Promise.all( | ||
transactionsAdded | ||
// Only queued transactions with queueNonce >= current txNonce | ||
.filter(({ args }) => args.queueNonce.gte(txNonce)) | ||
.map(async (event) => { | ||
const txBlock = await event.getBlock() | ||
|
||
const validFrom = BigNumber.from(txBlock.timestamp).add(txCooldown) | ||
const expiresAt = txExpiration.isZero() | ||
? null // Never expires | ||
: validFrom.add(txExpiration) | ||
|
||
return { | ||
...event, | ||
timestamp: txBlock.timestamp, | ||
validFrom, | ||
expiresAt, | ||
} | ||
}), | ||
) |
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 would extract this into its own function.
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've extracted it and added test coverage for it (and all other loading related logic) in 181b21d.
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.
Some unit tests would be good.
cy.get('p').contains('1').should('exist') | ||
cy.get('p').contains('2').should('exist') |
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 was failing as in #2768 (comment).
export const _getQueuedTransactionsAdded = ( | ||
transactionsAdded: Array<TransactionAddedEvent>, | ||
txNonce: BigNumber, | ||
): Array<TransactionAddedEvent> => { | ||
// Only queued transactions with queueNonce >= current txNonce | ||
return transactionsAdded.filter(({ args }) => args.queueNonce.gte(txNonce)) | ||
} | ||
|
||
export const _getRecoveryQueueItem = async ( | ||
transactionAdded: TransactionAddedEvent, | ||
txCooldown: BigNumber, | ||
txExpiration: BigNumber, | ||
): Promise<RecoveryQueueItem> => { | ||
const txBlock = await transactionAdded.getBlock() | ||
|
||
const validFrom = BigNumber.from(txBlock.timestamp).add(txCooldown) | ||
const expiresAt = txExpiration.isZero() | ||
? null // Never expires | ||
: validFrom.add(txExpiration) | ||
|
||
return { | ||
...transactionAdded, | ||
timestamp: txBlock.timestamp, | ||
validFrom, | ||
expiresAt, | ||
} | ||
} | ||
|
||
export const _getRecoveryState = async (delayModifier: Delay): Promise<RecoveryState[number]> => { | ||
const transactionAddedFilter = delayModifier.filters.TransactionAdded() | ||
|
||
const [[modules], txExpiration, txCooldown, txNonce, queueNonce, transactionsAdded] = await Promise.all([ | ||
delayModifier.getModulesPaginated(SENTINEL_ADDRESS, MAX_PAGE_SIZE), | ||
delayModifier.txExpiration(), | ||
delayModifier.txCooldown(), | ||
delayModifier.txNonce(), | ||
delayModifier.queueNonce(), | ||
delayModifier.queryFilter(transactionAddedFilter), | ||
]) | ||
|
||
const queuedTransactionsAdded = _getQueuedTransactionsAdded(transactionsAdded, txNonce) | ||
|
||
const queue = await Promise.all( | ||
queuedTransactionsAdded.map((transactionAdded) => | ||
_getRecoveryQueueItem(transactionAdded, txCooldown, txExpiration), | ||
), | ||
) | ||
|
||
return { | ||
address: delayModifier.address, | ||
modules, | ||
txExpiration, | ||
txCooldown, | ||
txNonce, | ||
queueNonce, | ||
queue, | ||
} | ||
} |
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.
One might ask at this point why they are in the hook and not in the services/recovery, but up to you.
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 moved the parsing logic to the @/services/recovery
folder in d646122.
delayModifier.txCooldown(), | ||
delayModifier.txNonce(), | ||
delayModifier.queueNonce(), | ||
delayModifier.queryFilter(transactionAddedFilter), |
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 think on some networks the size of logs that can be queried is limited.
And in general it is adviced to query the on-chain data in chunks (see here)
A good first step could be to at least limit the starting block to the block in which the Safe was deployed or the module was enabled. And for later polls we could remember the last checked block and start polling from there.
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.
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.
LGTM! We can tackle the optimization in a separate issue :)
What it solves
Resolves #2752
How this PR fixes it
This adds a new
recovery
slice to Redux. When a Safe is opened, and subsequently every ten minutes, the modules of it are parsed and for every Delay Modifier, details loaded.How to test it
Open a Safe with a Delay Modifier enabled, e.g.
gor:0xAecDFD3A19f777F0c03e6bf99AAfB59937d6467b
and observe the state load in Redux.Screenshots
Checklist