fix: AdminWallet.init() crash when no admin wallet is funded - #575
Open
goodbounties-nanoclaw-agent[bot] wants to merge 6 commits into
Open
fix: AdminWallet.init() crash when no admin wallet is funded#575goodbounties-nanoclaw-agent[bot] wants to merge 6 commits into
goodbounties-nanoclaw-agent[bot] wants to merge 6 commits into
Conversation
…ne are funded AdminWallet.init() left this.address undefined when every configured wallet was below the funding threshold, which later crashed the process with an invalid-address error on the next getBalance/getTransactionCount call. Fall back to the first configured address so the existing low-funds Slack alert can fire without taking the whole process down. Fixes #574 On-Behalf-Of: gdpatchagent[onecli] (yaskkeryodtdijpv)
…s funded Regression test for #574 - verifies AdminWallet.init() no longer crashes the process (via process.exit) when every configured wallet is below the funding threshold, and that it falls back to the first configured address instead of leaving address undefined. On-Behalf-Of: gdpatchagent[onecli] (yaskkeryodtdijpv)
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider resetting
filledAddressesinsideAdminWallet.init()rather than manipulating it externally in the test, so the initializer’s state management is self-contained and less coupled to test setup. - The new fallback
this.address = this.addresses[0]assumes at least one configured address; it may be worth guarding against an emptyaddressesarray to avoid a new crash path due to misconfiguration. - The regression test currently mocks
isVerifiedAdminto always returnfalse, which exercises the ‘no verified admin’ path rather than ‘all wallets underfunded’; if the distinction becomes important later, you may want a test that keeps them verified but under the balance threshold instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider resetting `filledAddresses` inside `AdminWallet.init()` rather than manipulating it externally in the test, so the initializer’s state management is self-contained and less coupled to test setup.
- The new fallback `this.address = this.addresses[0]` assumes at least one configured address; it may be worth guarding against an empty `addresses` array to avoid a new crash path due to misconfiguration.
- The regression test currently mocks `isVerifiedAdmin` to always return `false`, which exercises the ‘no verified admin’ path rather than ‘all wallets underfunded’; if the distinction becomes important later, you may want a test that keeps them verified but under the balance threshold instead.
## Individual Comments
### Comment 1
<location path="src/server/blockchain/Web3Wallet.js" line_range="409-410" />
<code_context>
+ // getBalance/getTransactionCount calls below crash the process on an invalid
+ // (undefined) address even though the low-funds condition is already alerted on
+ // via Slack a few lines down.
+ if (!this.address) {
+ this.address = this.addresses[0]
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against empty or undefined `this.addresses` when falling back.
If `this.addresses` is empty or unset, `this.addresses[0]` will still be undefined (or throw), so the later balance/transaction calls can still crash. Please add a defensive check around the fallback (e.g. `if (!this.address && this.addresses?.length) { ... }`) and explicitly handle the "no addresses configured" case so those calls never receive an invalid address.
</issue_to_address>
### Comment 2
<location path="src/server/blockchain/__tests__/adminWallet.test.js" line_range="146-163" />
<code_context>
await release()
})
+
+ test('adminWallet init falls back to a default address instead of crashing when no wallet is funded', async () => {
+ const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {})
+ const isVerifiedAdminSpy = jest.spyOn(AdminWallet, 'isVerifiedAdmin').mockResolvedValue(false)
+
+ // init() appends to filledAddresses instead of resetting it, so clear out whatever
+ // the real (unmocked) init from beforeAll already populated before re-running it here.
+ AdminWallet.filledAddresses = []
+ AdminWallet.address = undefined
+
+ await AdminWallet.init()
+
+ expect(AdminWallet.address).toBe(AdminWallet.addresses[0])
+ expect(AdminWallet.filledAddresses).toEqual([])
+ expect(exitSpy).not.toHaveBeenCalled()
+
+ isVerifiedAdminSpy.mockRestore()
+ exitSpy.mockRestore()
+ })
})
</code_context>
<issue_to_address>
**suggestion (testing):** Use try/finally around mocks to guarantee cleanup even if the test fails.
Currently `mockRestore()` is only reached if all assertions pass. If an earlier assertion throws, the mocked `process.exit` and `AdminWallet.isVerifiedAdmin` will persist into later tests. Please wrap the test body in `try { ... } finally { isVerifiedAdminSpy.mockRestore(); exitSpy.mockRestore(); }` so mocks are always restored, even on failure, and don’t leak between tests.
```suggestion
test('adminWallet init falls back to a default address instead of crashing when no wallet is funded', async () => {
const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {})
const isVerifiedAdminSpy = jest
.spyOn(AdminWallet, 'isVerifiedAdmin')
.mockResolvedValue(false)
try {
// init() appends to filledAddresses instead of resetting it, so clear out whatever
// the real (unmocked) init from beforeAll already populated before re-running it here.
AdminWallet.filledAddresses = []
AdminWallet.address = undefined
await AdminWallet.init()
expect(AdminWallet.address).toBe(AdminWallet.addresses[0])
expect(AdminWallet.filledAddresses).toEqual([])
expect(exitSpy).not.toHaveBeenCalled()
} finally {
isVerifiedAdminSpy.mockRestore()
exitSpy.mockRestore()
}
})
```
</issue_to_address>
### Comment 3
<location path="src/server/blockchain/__tests__/adminWallet.test.js" line_range="155-159" />
<code_context>
+ AdminWallet.filledAddresses = []
+ AdminWallet.address = undefined
+
+ await AdminWallet.init()
+
+ expect(AdminWallet.address).toBe(AdminWallet.addresses[0])
+ expect(AdminWallet.filledAddresses).toEqual([])
+ expect(exitSpy).not.toHaveBeenCalled()
+
+ isVerifiedAdminSpy.mockRestore()
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for the case where there are no configured admin addresses at all.
Also consider a test where `AdminWallet.addresses` is empty (no configured admin wallets). That will document how `init()` behaves when the fallback address is unavailable and guard against reintroducing crashes in that scenario.
Suggested implementation:
```javascript
test('adminWallet init falls back to a default address instead of crashing when no wallet is funded', async () => {
const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {})
const isVerifiedAdminSpy = jest.spyOn(AdminWallet, 'isVerifiedAdmin').mockResolvedValue(false)
// init() appends to filledAddresses instead of resetting it, so clear out whatever
// the real (unmocked) init from beforeAll already populated before re-running it here.
AdminWallet.filledAddresses = []
AdminWallet.address = undefined
await AdminWallet.init()
expect(AdminWallet.address).toBe(AdminWallet.addresses[0])
expect(AdminWallet.filledAddresses).toEqual([])
expect(exitSpy).not.toHaveBeenCalled()
isVerifiedAdminSpy.mockRestore()
exitSpy.mockRestore()
})
test('adminWallet init handles the case where there are no configured admin addresses at all', async () => {
const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {})
// Preserve the original configured addresses so we can restore them after the test.
const originalAddresses = AdminWallet.addresses
// Simulate a configuration where there are no admin addresses configured at all.
// This documents how init() behaves when the configuration does not provide any
// admin wallets.
AdminWallet.addresses = undefined
AdminWallet.filledAddresses = []
AdminWallet.address = undefined
await AdminWallet.init()
// When no admin addresses are configured at all, init() should not crash the test
// runner, but it is expected to fail fast. We assert that process.exit is invoked
// to document and guard this behavior.
expect(exitSpy).toHaveBeenCalled()
expect(AdminWallet.address).toBeUndefined()
expect(AdminWallet.filledAddresses).toEqual([])
// Restore original state so other tests are not affected.
AdminWallet.addresses = originalAddresses
exitSpy.mockRestore()
})
test('adminWallet init behaves correctly when AdminWallet.addresses is empty (no configured admin wallets)', async () => {
const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {})
const originalAddresses = AdminWallet.addresses
// Simulate a configuration where the admin addresses array exists but is empty.
// This documents how init() behaves when the fallback address is unavailable.
AdminWallet.addresses = []
AdminWallet.filledAddresses = []
AdminWallet.address = undefined
await AdminWallet.init()
// With an empty addresses list, there is no fallback address available. We assert
// that init() fails in a controlled way and does not throw uncaught exceptions.
expect(exitSpy).toHaveBeenCalled()
expect(AdminWallet.address).toBeUndefined()
expect(AdminWallet.filledAddresses).toEqual([])
AdminWallet.addresses = originalAddresses
exitSpy.mockRestore()
})
})
```
If the actual expected behavior for `AdminWallet.init()` in the "no addresses" scenarios is different (e.g. it should *not* call `process.exit` and instead log an error or choose another fallback), you should adjust the `expect(exitSpy).toHaveBeenCalled()` / `not.toHaveBeenCalled()` assertions accordingly to reflect the true contract of `init()`. Also ensure that manipulating `AdminWallet.addresses` directly is consistent with how the rest of the test suite configures admin wallets; if there is a helper or configuration loader used elsewhere, you may want to use that instead of assigning to `AdminWallet.addresses` manually.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+146
to
+163
| test('adminWallet init falls back to a default address instead of crashing when no wallet is funded', async () => { | ||
| const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {}) | ||
| const isVerifiedAdminSpy = jest.spyOn(AdminWallet, 'isVerifiedAdmin').mockResolvedValue(false) | ||
|
|
||
| // init() appends to filledAddresses instead of resetting it, so clear out whatever | ||
| // the real (unmocked) init from beforeAll already populated before re-running it here. | ||
| AdminWallet.filledAddresses = [] | ||
| AdminWallet.address = undefined | ||
|
|
||
| await AdminWallet.init() | ||
|
|
||
| expect(AdminWallet.address).toBe(AdminWallet.addresses[0]) | ||
| expect(AdminWallet.filledAddresses).toEqual([]) | ||
| expect(exitSpy).not.toHaveBeenCalled() | ||
|
|
||
| isVerifiedAdminSpy.mockRestore() | ||
| exitSpy.mockRestore() | ||
| }) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Use try/finally around mocks to guarantee cleanup even if the test fails.
Currently mockRestore() is only reached if all assertions pass. If an earlier assertion throws, the mocked process.exit and AdminWallet.isVerifiedAdmin will persist into later tests. Please wrap the test body in try { ... } finally { isVerifiedAdminSpy.mockRestore(); exitSpy.mockRestore(); } so mocks are always restored, even on failure, and don’t leak between tests.
Suggested change
| test('adminWallet init falls back to a default address instead of crashing when no wallet is funded', async () => { | |
| const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {}) | |
| const isVerifiedAdminSpy = jest.spyOn(AdminWallet, 'isVerifiedAdmin').mockResolvedValue(false) | |
| // init() appends to filledAddresses instead of resetting it, so clear out whatever | |
| // the real (unmocked) init from beforeAll already populated before re-running it here. | |
| AdminWallet.filledAddresses = [] | |
| AdminWallet.address = undefined | |
| await AdminWallet.init() | |
| expect(AdminWallet.address).toBe(AdminWallet.addresses[0]) | |
| expect(AdminWallet.filledAddresses).toEqual([]) | |
| expect(exitSpy).not.toHaveBeenCalled() | |
| isVerifiedAdminSpy.mockRestore() | |
| exitSpy.mockRestore() | |
| }) | |
| test('adminWallet init falls back to a default address instead of crashing when no wallet is funded', async () => { | |
| const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {}) | |
| const isVerifiedAdminSpy = jest | |
| .spyOn(AdminWallet, 'isVerifiedAdmin') | |
| .mockResolvedValue(false) | |
| try { | |
| // init() appends to filledAddresses instead of resetting it, so clear out whatever | |
| // the real (unmocked) init from beforeAll already populated before re-running it here. | |
| AdminWallet.filledAddresses = [] | |
| AdminWallet.address = undefined | |
| await AdminWallet.init() | |
| expect(AdminWallet.address).toBe(AdminWallet.addresses[0]) | |
| expect(AdminWallet.filledAddresses).toEqual([]) | |
| expect(exitSpy).not.toHaveBeenCalled() | |
| } finally { | |
| isVerifiedAdminSpy.mockRestore() | |
| exitSpy.mockRestore() | |
| } | |
| }) |
Comment on lines
+155
to
+159
| await AdminWallet.init() | ||
|
|
||
| expect(AdminWallet.address).toBe(AdminWallet.addresses[0]) | ||
| expect(AdminWallet.filledAddresses).toEqual([]) | ||
| expect(exitSpy).not.toHaveBeenCalled() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for the case where there are no configured admin addresses at all.
Also consider a test where AdminWallet.addresses is empty (no configured admin wallets). That will document how init() behaves when the fallback address is unavailable and guard against reintroducing crashes in that scenario.
Suggested implementation:
test('adminWallet init falls back to a default address instead of crashing when no wallet is funded', async () => {
const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {})
const isVerifiedAdminSpy = jest.spyOn(AdminWallet, 'isVerifiedAdmin').mockResolvedValue(false)
// init() appends to filledAddresses instead of resetting it, so clear out whatever
// the real (unmocked) init from beforeAll already populated before re-running it here.
AdminWallet.filledAddresses = []
AdminWallet.address = undefined
await AdminWallet.init()
expect(AdminWallet.address).toBe(AdminWallet.addresses[0])
expect(AdminWallet.filledAddresses).toEqual([])
expect(exitSpy).not.toHaveBeenCalled()
isVerifiedAdminSpy.mockRestore()
exitSpy.mockRestore()
})
test('adminWallet init handles the case where there are no configured admin addresses at all', async () => {
const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {})
// Preserve the original configured addresses so we can restore them after the test.
const originalAddresses = AdminWallet.addresses
// Simulate a configuration where there are no admin addresses configured at all.
// This documents how init() behaves when the configuration does not provide any
// admin wallets.
AdminWallet.addresses = undefined
AdminWallet.filledAddresses = []
AdminWallet.address = undefined
await AdminWallet.init()
// When no admin addresses are configured at all, init() should not crash the test
// runner, but it is expected to fail fast. We assert that process.exit is invoked
// to document and guard this behavior.
expect(exitSpy).toHaveBeenCalled()
expect(AdminWallet.address).toBeUndefined()
expect(AdminWallet.filledAddresses).toEqual([])
// Restore original state so other tests are not affected.
AdminWallet.addresses = originalAddresses
exitSpy.mockRestore()
})
test('adminWallet init behaves correctly when AdminWallet.addresses is empty (no configured admin wallets)', async () => {
const exitSpy = jest.spyOn(process, 'exit').mockImplementation(() => {})
const originalAddresses = AdminWallet.addresses
// Simulate a configuration where the admin addresses array exists but is empty.
// This documents how init() behaves when the fallback address is unavailable.
AdminWallet.addresses = []
AdminWallet.filledAddresses = []
AdminWallet.address = undefined
await AdminWallet.init()
// With an empty addresses list, there is no fallback address available. We assert
// that init() fails in a controlled way and does not throw uncaught exceptions.
expect(exitSpy).toHaveBeenCalled()
expect(AdminWallet.address).toBeUndefined()
expect(AdminWallet.filledAddresses).toEqual([])
AdminWallet.addresses = originalAddresses
exitSpy.mockRestore()
})
})If the actual expected behavior for AdminWallet.init() in the "no addresses" scenarios is different (e.g. it should not call process.exit and instead log an error or choose another fallback), you should adjust the expect(exitSpy).toHaveBeenCalled() / not.toHaveBeenCalled() assertions accordingly to reflect the true contract of init(). Also ensure that manipulating AdminWallet.addresses directly is consistent with how the rest of the test suite configures admin wallets; if there is a helper or configuration loader used elsewhere, you may want to use that instead of assigning to AdminWallet.addresses manually.
- Guard against an empty/misconfigured addresses list instead of silently falling back to an undefined address (throws a clear error and hits the existing catch/exit path). - Reset filledAddresses at the top of each init() run so re-running init() does not keep accumulating addresses from a previous run. On-Behalf-Of: gdpatchagent[onecli] (yaskkeryodtdijpv)
Wrap the regression test body in try/finally so the process.exit and isVerifiedAdmin mocks are always restored, even if an assertion throws, so they cannot leak into later tests. On-Behalf-Of: gdpatchagent[onecli] (yaskkeryodtdijpv)
Author
|
Addressed all three points:
Left the "verified-but-underfunded vs not-verified" test-distinction suggestion out of scope for this fix - the current test already exercises the crash path this PR fixes; happy to add that as a separate follow-up if useful. Full adminWallet.test.js suite (13/13) re-verified locally against a hardhat node + deployed contracts, lint clean. |
sirpy
requested changes
Aug 17, 2026
sirpy
left a comment
sirpy
left a comment
Contributor
There was a problem hiding this comment.
simply remove the process.exit and send a more urgent message to slack
Contributor
…erfunded-crash Co-authored-by: L03TJ3 <6606028+L03TJ3@users.noreply.github.com>
…init() catch block Co-authored-by: L03TJ3 <6606028+L03TJ3@users.noreply.github.com>
Contributor
Applied @sirpy's feedback and merged with master (clean merge, only
Done in the latest commit. Removed |
Contributor
|
@L03TJ3 do we need the other changes? |
Contributor
|
only if this review/case makes sense to additionally handle: When every configured admin wallet is underfunded (your "no fuse admin wallet with funds" case), that condition never hits, so this.address stays undefined. The code correctly detects filledAddresses.length === 0 and sends the Slack alert (that part works) — but then keeps going. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #574
Problem
GoodServer crashes (process.exit) when every configured admin wallet address is below
the funding threshold.
AdminWallet.init()only setsthis.addressinside the loop thatrequires an address to both be a verified admin and above
minAdminBalance. When noaddress qualifies,
this.addressstaysundefinedand is later passed toweb3.eth.getBalance()/getTransactionCount(), which throws an invalid-address error.That error is caught by the outer try/catch and triggers
process.exit(-1), crashing thewhole process - even though the existing "no fuse admin wallet with funds" Slack alert had
already fired a few lines below.
Fix
Fall back to the first configured address (
this.addresses[0]) when no wallet meets thefunding threshold, so initialization completes and the process stays up. The alerting logic
and the fatal-error-exit behavior for genuine errors are unchanged.
Verification
isVerifiedAdmin)and confirmed it, plus the full existing
adminWallet.test.jssuite (13/13), pass against alocal hardhat + contract deploy.
Summary by Sourcery
Prevent AdminWallet initialization from crashing when no configured admin wallet meets the funding threshold by ensuring a valid fallback address is always set.
Bug Fixes:
Tests: