Normalize empty, not found, null outputs

[hugomrdias]

We need to normalize contract output for empty, not found states, behavior varies intra-contract and between contracts.

• do it in sdk ? core ?
• in sdk return
• throw NotFoundError

ref:

• https://www.soliditylang.org/blog/2021/04/21/custom-errors/
• https://viem.sh/docs/contract/simulateContract#handling-custom-errors

Also, require(condition, "error message") should be translated to if (!condition) revert CustomError().

/cc @rvagg

[wjmelements]

In ts, I'd prefer to return a falsy value for null nullables instead of throwing and catching, because catching can mask other errors as it has in the past. Exceptions should be used for unexpected errors. Lookup methods can return A | null. For example, I don't think ping, selectProviderWithPing, or selectRandomProvider should throw, but I do agree with throwing on 'Operation aborted' and 'Invalid PieceCID provided'.

In solidity, I want to throw (revert) if the normal return type isn't a valid response, because success/failure is checked by default for every call, and because solidity is strongly-typed. We also want to use strongly-typed ABI errors instead of strings, and for the most part we already do. If you see legacy string errors in the codebase, please open a PR to replace them.

[rvagg]

I'm not quite clear on the question here @hugomrdias, is this about empty/zero values for getters like your question and title suggests, or about custom error returns like your links suggest?

Regarding not found: ISTM that we're bridging two worlds here - in the Solidity world, "not found" often means returning a zero value, but in JS the behaviour is either to throw or return undefined, depending on what's being done. For simple accessors/getters, undefined is more common, so I'd be fine with that pattern for dealing with those cases, as long as we're clear about this with types and the behaviour bubbles up to the point where it matters so we can potentially throw if we expected there to be something.

Regarding custom errors: yeah, we have a ton of inconsistency in our Solidity, some of them return nice error shapes and others are just strings. It would be nice to normalise them all into consistent errors that can be handled, I think we're now doing a lot of that by doing selector translation. I don't want to spent gobs of time chasing down all of the cases but in terms of default approach I like normalising to typed errors, but as long as you get some kind of error then that's better than nothing.

[hugomrdias]

First I would like typed errors in solidity to improve handling in js. Right now we have lots of error.message.includes('not found') which sometimes does even work because there's a second error string in solidity.

Second, return null vs throw empty error.
I'm good with returning undefined we just need typed errors to be able to catch and just rethrow the others.
There's also instances in solidity that empty state is zero value and others error string and probably others typed errors (not confirmed this last one)

[rvagg]

Well sir, here's your analysis of the work we'd need to do:

Summary

Contract	Untyped require("string")	Untyped revert("string")	No-message require()	Has Errors.sol	Upgradeable
filecoin-pay
FilecoinPayV1	0	0	0	Yes (60+)	No
RateChangeQueue (library)	3	0	0	Parent has	No
pdp
PDPVerifier	41	2	4	No	Yes
SimplePDPService (not used by FOC)	3	10	0	No	-
Cids.sol (library)	3	2	0	No	Yes
Fees.sol (library)	1	0	0	No	Yes
Proofs.sol (library)	4	0	0	No	Yes
BitOps.sol (library)	1	0	0	No	Yes
filecoin-services
ServiceProviderRegistry	30	1	4	Yes (unused)	Yes
FilecoinWarmStorageService	1	1	4	Yes (90+)	Yes
SignatureVerificationLib (library)	1	0	0	Yes	Yes
Total	88	16	12

---

PDPVerifier

File: src/PDPVerifier.sol
Errors.sol: Does not exist for PDP contracts

No-message requires (4)

Line	Code
185	require(plannedUpgrade.nextImplementation.code.length > 3000);
186	require(plannedUpgrade.afterEpoch > block.number);
193	require(newImplementation == nextUpgrade.nextImplementation);
194	require(block.number >= nextUpgrade.afterEpoch);

String requires (41)

Line	Error String
199	"Incorrect fee amount"
201	"Burn failed"
208	"sybil fee not met"
217	"Transfer failed."
254	"Data set not live"
260	"Data set not live"
266	"Data set not live"
272	"Data set not live"
278	"Data set not live"
283	"Data set not live"
289	"Data set not live"
295	"Data set not live"
301	"Data set not live"
307	"Data set not live"
322	"Data set not live"
346	"Data set not live"
347	"Limit must be greater than 0"
396	"Data set not live"
410	"Data set not live"
469	"Only the storage provider can delete data sets"
515	"listener required for new dataset"
527	"listener must be zero for existing dataset"
528	"no fee on add to existing dataset"
530	"Data set not live"
531	"Only the storage provider can add pieces"
543	"Must add at least one piece"
587	"Data set not live"
588	"Only the storage provider can schedule removal of pieces"
596	"Can only schedule removal of existing pieces"
597	"Can only schedule removal of live pieces"
626	"Only the storage provider can prove possession"
627	"empty proof"
630	"premature proof"
631	"no challenge scheduled"
663	"proof did not verify"
688	"Transfer failed."
700	"failed to validate: proof size must be greater than 0"
754	"only the storage provider can move to next proving period"
755	"can only start proving once leaves are added"
810	"Data set not live"
889	"Leaf index out of bounds"

String reverts (2)

Line	Error String
466	"data set id out of bounds"
787	"challenge epoch must be at least challengeFinality epochs in the future"

---

SimplePDPService

Not used in FOC

File: src/SimplePDPService.sol
Errors.sol: Does not exist

String requires (3)

Line	Error String
60	"Event index out of bounds"
95	"PDP verifier address cannot be zero"
103	"Caller is not the PDP verifier"

String reverts (10)

Line	Error String
128	"Proving period not yet initialized"
146	"Proving period not yet initialized"
223	"Only one proof of possession allowed per proving period. Open a new proving period."
226	"Invalid challenge count < 5"
229	"Proving not yet started"
233	"Current proving period passed. Open a new proving period."
237	"Too early. Wait for challenge window to open"
255	"Next challenge epoch must fall within the next challenge window"
266	"One call to nextProvingPeriod allowed per proving period"
285	"Next challenge epoch must fall within the next challenge window"

---

Cids.sol

File: src/Cids.sol

String requires (3)

Line	Error String
16	"Cid data is too short"
26	"Cid data is too short"
45	"CommPv2 multihash length must be at least 34"

String reverts (2)

Line	Error String
39	"Cid must be CommPv2"
47	"CommPv2 multihash length does not match data length"

---

Fees.sol

File: src/Fees.sol

String requires (1)

Line	Error String
26	"failed to validate: raw size must be greater than 0"

---

Proofs.sol

File: src/Proofs.sol

String requires (4)

Line	Error String
32	"proof length does not match tree height"
70	"Height must be <= 50"
136	"Leaves array must not be empty"
165	"Index out of bounds"

---

BitOps.sol

File: src/BitOps.sol

String requires (1)

Line	Error String
62	"Input exceeds maximum int256 value"

---

RateChangeQueue

File: src/RateChangeQueue.sol

String requires (3)

Line	Error String
23	"Queue is empty"
41	"Queue is empty"
46	"Queue is empty"

---

ServiceProviderRegistry

File: service_contracts/src/ServiceProviderRegistry.sol
Errors.sol: Exists at src/Errors.sol but largely unused

No-message requires (4)

Line	Code
775	require(plannedUpgrade.nextImplementation.code.length > 3000);
776	require(plannedUpgrade.afterEpoch > block.number);
787	require(newImplementation == nextUpgrade.nextImplementation);
788	require(block.number >= nextUpgrade.afterEpoch);

String requires (30)

Line	Error String
122	"Only service provider can call this function"
128	"Provider does not exist"
129	"Provider not found"
135	"Provider is not active"
172	"Only PDP product type currently supported"
175	"Payee cannot be zero address"
178	"Address already registered"
181	"Incorrect fee amount"
184	"Name too long"
187	"Description too long"
216	"Burn failed"
227	"Only PDP product type currently supported"
230	"Provider not registered"
243	"Product already exists for this provider"
288	"Only PDP product type currently supported"
291	"Provider not registered"
307	"Product does not exist for this provider"
339	"Only PDP product type currently supported"
342	"Provider not registered"
355	"Product does not exist for this provider"
381	"Provider not registered"
382	"Provider does not exist"
383	"Provider not found"
384	"Provider is not active"
387	"Name too long"
390	"Description too long"
403	"Provider not registered"
760	"Keys and values arrays must have same length"
761	"Too many capabilities"
764	"Capability key cannot be empty"
765	"Capability key too long"
766	"Capability value cannot be empty"
767	"Capability value too long"

String reverts (1)

Line	Error String
743	"Unsupported product type"

---

FilecoinWarmStorageService

File: service_contracts/src/FilecoinWarmStorageService.sol
Errors.sol: Exists and is well-used (90+ typed errors)

No-message requires (4)

Line	Code
430	require(plannedUpgrade.nextImplementation.code.length > 3000);
431	require(plannedUpgrade.afterEpoch > block.number);
438	require(newImplementation == nextUpgrade.nextImplementation);
439	require(block.number >= nextUpgrade.afterEpoch);

String requires (1)

Line	Error String
570	"Provider ID mismatch at index"

String reverts (1)

Line	Error String
1064	"Storage provider changes are not yet supported"

---

SignatureVerificationLib

File: service_contracts/src/lib/SignatureVerificationLib.sol
Errors.sol: Uses parent Errors.sol

String requires (1)

Line	Error String
130	"Keys/values array length mismatch"

Note: Line 58 has the same validation but uses typed Errors.MetadataKeyAndValueLengthMismatch(). This is an inconsistency.

[wjmelements]

String errors might be also be abi errors. I think they are encoded as Error(string) (0x08c379a0).