fix(api): allow GET /wallet when swap or chequebook is disabled#5468
fix(api): allow GET /wallet when swap or chequebook is disabled#5468martinconic wants to merge 1 commit into
Conversation
martinconic
requested review from
acud,
akrem-chabchoub,
gacevicljubisa,
janos and
sbackend123
There was a problem hiding this comment.
Nice fix.
We can consider addressing one TODO to add BzzAddress to ChainConfig and drop LookupERC20Address entirely. The BZZ token address is deterministic per chain, and no reason to lok it up at runtime. It is maybe out of scope here, but maybe we can make an additional PR? Or maybe even in this one? Wdyt @acud @janos ?
| if err != nil { | ||
| return nil, fmt.Errorf("init chequebook factory: %w", err) | ||
| if chainEnabled { | ||
| chequebookFactory, ferr := InitChequebookFactory(logger, chainBackend, chainID, transactionService, o.SwapFactoryAddress) |
There was a problem hiding this comment.
When swap is disabled, the factory is only being initialized so we can read the BZZ token address. Could we use postagecontract.LookupERC20Address above this instead?
LookupERC20Address resolves the same token from the postage stamp contract, and would let us skip InitChequebookFactory entirely on the swap disabled path?
There was a problem hiding this comment.
Good catch — postagecontract.LookupERC20Address is in fact already called below in the postage setup (node.go:726) to resolve bzzTokenAddress, so the factory's ERC20Address here is redundant on the swap-disabled path. Reusing it would let us drop the factory init entirely when swap is off. The wrinkle is ordering: erc20Service is built here, while bzzTokenAddress only resolves ~180 lines lower, so switching cleanly means reordering the erc20 init relative to the postage block. I'd rather keep this PR scoped to the /wallet gating fix and do the lookup reuse as a follow-up so the reordering gets its own review. WDYT?
| chequebookFactory, err := InitChequebookFactory(logger, chainBackend, chainID, transactionService, o.SwapFactoryAddress) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("init chequebook factory: %w", err) | ||
| if chainEnabled { |
There was a problem hiding this comment.
Consider extracting this whole chainEnabled block into a private helper, there are to many conditionals, it is hard to follow...
There was a problem hiding this comment.
I agree, and we would need some refactoring in general not only for chainEnabled here, chainEnabled and other options are hard to follow in multiple areas. This should be a different PR and some effort is already ongoing in #5471
This is a good suggestion and task , I would make this separately and keep this PR light, covering only the scope of the reported issue. |
2 participants
Checklist
Description
The /wallet endpoint was gated behind both swap and chequebook
availability, and erc20Service was only initialized when swap was
enabled. As a result, node operators could not read their xDAI/xBZZ
balances with swap-enable=false or chequebook-enable=false.
Initialize the chequebook factory and erc20Service whenever the chain
backend is enabled, independently of swap/chequebook. The lookup stays
fail-hard when swap is enabled (preserving existing startup behaviour)
and is best-effort when swap is off, so nodes on custom chains without a
known factory address still start. Gate /wallet and /wallet/withdraw on
chain availability instead of swap/chequebook, and make the handler
tolerate a nil erc20Service.
Add an explicit startup guard rejecting swap-enable=true with the chain
backend disabled, which previously failed deep in the chequebook factory
call; without it the new flow would leave swap nil and panic on
/settlements and related endpoints.
Closes #5233
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):
AI Disclosure