Skip to content

fix(txm): nonce gap #561

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

Merged
GabrielMartinezRodriguez merged 5 commits into from
Apr 10, 2025
Merged

fix(txm): nonce gap #561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a2f4c98
fix(txm): nonce gap
GabrielMartinezRodriguez Mar 25, 2025
ac02e17
fix(contracts): deploy randomness
GabrielMartinezRodriguez Apr 8, 2025
3e1921c
chore(contracts): added json flag to post-deploy rule
GabrielMartinezRodriguez Apr 10, 2025
d03ec85
chore(contracts): update mock address
GabrielMartinezRodriguez Apr 10, 2025
de3af6c
chore(contracts): update mock address
GabrielMartinezRodriguez Apr 10, 2025
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
12 changes: 6 additions & 6 deletions contracts/deployments/anvil/mocks/abis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,12 +440,12 @@ const aliasToContract = ({
}) as const

export const deployment = ({
"HappyCounter": "0x8D45cAd49F37CC512DAFFE6700ddc98084867E68",
"MockGasBurner": "0xdA504Bb1b736b04A5Aec28fD5d693Ad7447Ad438",
"MockRevert": "0x4065fA94A420c30c9bBc32483e65CaC8edDDa855",
"MockTokenA": "0xA41Be5C0a84e4e62273D2D1138456d069F897913",
"MockTokenB": "0xC886E11da9747684B36FA0E9519539A16cB1739a",
"MockTokenC": "0x40C7343b1Ed89bc85D914239154F662Ed6e028Ed"
"HappyCounter": "0xCeC57308B882Cf2A770ed57573B09d77a280b92F",
"MockGasBurner": "0xB37ABfb788e71bb93171ec06F237a05F954E5F85",
"MockRevert": "0x1015B0D8fB2662C1c1A4f3cEE38054ff6d8117Bc",
"MockTokenA": "0x20F24e61ae939B180293E65D54aB8B5941011381",
"MockTokenB": "0x77e1acd06c1eF93441F159c11C2d1d644a460e85",
"MockTokenC": "0x1B9a60C271401De40E90d0100Ab8881B2FaF7210"
}) as const

export type ContractToAbi = typeof contractToAbi
Expand Down
12 changes: 6 additions & 6 deletions contracts/deployments/anvil/mocks/deployment.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"HappyCounter": "0x8D45cAd49F37CC512DAFFE6700ddc98084867E68",
"MockGasBurner": "0xdA504Bb1b736b04A5Aec28fD5d693Ad7447Ad438",
"MockRevert": "0x4065fA94A420c30c9bBc32483e65CaC8edDDa855",
"MockTokenA": "0xA41Be5C0a84e4e62273D2D1138456d069F897913",
"MockTokenB": "0xC886E11da9747684B36FA0E9519539A16cB1739a",
"MockTokenC": "0x40C7343b1Ed89bc85D914239154F662Ed6e028Ed"
"HappyCounter": "0xCeC57308B882Cf2A770ed57573B09d77a280b92F",
"MockGasBurner": "0xB37ABfb788e71bb93171ec06F237a05F954E5F85",
"MockRevert": "0x1015B0D8fB2662C1c1A4f3cEE38054ff6d8117Bc",
"MockTokenA": "0x20F24e61ae939B180293E65D54aB8B5941011381",
"MockTokenB": "0x77e1acd06c1eF93441F159c11C2d1d644a460e85",
"MockTokenC": "0x1B9a60C271401De40E90d0100Ab8881B2FaF7210"
}
6 changes: 3 additions & 3 deletions packages/txm/lib/NonceManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export class NonceManager {

const blockchainNonce = blockchainNonceResult.value

this.maxExecutedNonce = blockchainNonce
this.maxExecutedNonce = blockchainNonce - 1
Copy link
Contributor

@not-reed not-reed Mar 27, 2025

Choose a reason for hiding this comment

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

probably need to clamp this to not go below zero?

Copy link
Contributor

@aodhgan aodhgan Apr 1, 2025

Choose a reason for hiding this comment

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

agree, edge case for when nonce is 0 is unlikely but technically possible for a new txm instance

Copy link
Contributor Author

@GabrielMartinezRodriguez GabrielMartinezRodriguez Apr 10, 2025

Choose a reason for hiding this comment

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

It’s actually the expected behavior for it to be -1. If the user has an account that has never been used, the blockchain nonce will be 0, and therefore the max executed nonce will become -1. We use the max executed nonce to check if there is any pending transaction with a nonce lower than the maximum executed nonce, and if so, we move it to Interrupted.

if (nonce <= this.transactionManager.nonceManager.maxExecutedNonce) {
    transaction.changeStatus(TransactionStatus.Interrupted)
    return
}

So I think it’s fine for it to remain at -1 because it serves our purpose.

If we change it to undefined, I think the code is going to get messier, because we would have to handle the undefined case in several places, whereas -1 is fully functional


const highestDbNonce = this.txmgr.transactionRepository.getHighestNonce()

Expand All @@ -67,7 +67,7 @@ export class NonceManager {
} else {
this.nonce = highestDbNonce + 1
this.returnedNonceQueue = this.txmgr.transactionRepository
.getNotReservedNoncesInRange(blockchainNonce, highestDbNonce)
.getNotReservedNoncesInRange(this.maxExecutedNonce, highestDbNonce)
.sort((a, b) => a - b)
}
}
Expand Down Expand Up @@ -113,6 +113,6 @@ export class NonceManager {
return
}

this.maxExecutedNonce = blockchainNonceResult.value
this.maxExecutedNonce = blockchainNonceResult.value - 1
}
}
Loading