Skip to content
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

Bug: Library does not return errors for receive #130

Closed
callebtc opened this issue May 2, 2024 · 5 comments
Closed

Bug: Library does not return errors for receive #130

callebtc opened this issue May 2, 2024 · 5 comments

Comments

@callebtc
Copy link
Contributor

callebtc commented May 2, 2024

cashu-ts tries to receive multiple tokens from multiple mints at once and returns tokens with errors in receive. I think this is not a great approach because the implementor does not receive the error.

I am also not sure if the treatment of the counter for deterministic derivation still would work as intended. The counter should depend on the keyset which implies that a function using it should only create outputs of a single keyset, otherwise that function would need multiple counters.

I would suggest that the library should only receive tokens from a single mint with receive and multiple mints should be handled by an additional function. That means receive can return the error to the implementor. We should also check other functions that do not return errors and make them return it.

Any thoughts on this @gandlafbtc?

I'm c/p the relevant code here to make it easier to reason about:

	/**
	 * Receive an encoded or raw Cashu token
	 * @param {(string|Token)} token - Cashu token
	 * @param preference optional preference for splitting proofs into specific amounts
	 * @param counter? optionally set counter to derive secret deterministically. CashuWallet class must be initialized with seed phrase to take effect
	 * @param pubkey? optionally locks ecash to pubkey. Will not be deterministic, even if counter is set!
	 * @param privkey? will create a signature on the @param token secrets if set
	 * @returns New token with newly created proofs, token entries that had errors
	 */
	async receive(
		token: string | Token,
		options?: {
			preference?: Array<AmountPreference>;
			counter?: number;
			pubkey?: string;
			privkey?: string;
		}
	): Promise<ReceiveResponse> {
		let decodedToken: Array<TokenEntry>;
		if (typeof token === 'string') {
			decodedToken = cleanToken(getDecodedToken(token)).token;
		} else {
			decodedToken = token.token;
		}
		const tokenEntries: Array<TokenEntry> = [];
		const tokenEntriesWithError: Array<TokenEntry> = [];
		for (const tokenEntry of decodedToken) {
			if (!tokenEntry?.proofs?.length) {
				continue;
			}
			try {
				const { proofs, proofsWithError } = await this.receiveTokenEntry(tokenEntry, {
					preference: options?.preference,
					counter: options?.counter,
					pubkey: options?.pubkey,
					privkey: options?.privkey
				});
				if (proofsWithError?.length) {
					tokenEntriesWithError.push(tokenEntry);
					continue;
				}
				tokenEntries.push({ mint: tokenEntry.mint, proofs: [...proofs] });
			} catch (error) {
				console.error(error);
				tokenEntriesWithError.push(tokenEntry);
			}
		}
		return {
			token: { token: tokenEntries },
			tokensWithErrors: tokenEntriesWithError.length ? { token: tokenEntriesWithError } : undefined
		};
	}
@callebtc callebtc changed the title Bug: Library does not return errors for swap Bug: Library does not return errors for receive May 2, 2024
@Dayvvo
Copy link
Contributor

Dayvvo commented May 20, 2024

@callebtc looking at the I don't think the receive function not returning errors for tokens that failed to be redeemed is a result/ consequence of the receive function being able to accept tokens from different mints and as such doesn't in itself hinder errors with failed tokens being returned. I'm not particularly sure if all errors would be useful but if we wanted to return errors here we could have them returned from the receiveTokenEntry function being called in receive.

Secondly regarding the counter I think changing it's application might need fixes across multiple functions as it seems to be consistently used the same way across multiple functions in the CashuWallet class. So personally I think that might be a lot of work.

let me know your thoughts on this @callebtc @gandlafbtc

@gandlafbtc
Copy link
Collaborator

I think we should aim to simplify the datastructure of tokens to not allow multiple tokens inside a token.

Reasons:

  • less complexity and confusion
  • we can disregard this change, and return errors properly
  • if we must, we can achieve the same by chaining tokens and processing them sequentially

@gandlafbtc
Copy link
Collaborator

#86 has some additional discussion on this topic

@Dayvvo
Copy link
Contributor

Dayvvo commented Jun 12, 2024

@gandlafbtc as per the issue closed on #86 this issue can be closed out too, correct?

@gudnuf
Copy link
Contributor

gudnuf commented Jul 29, 2024

The api has been simplified and now either throws an error or returns the successfully swapped proofs. I think this can be closed @gandlafbtc

@callebtc callebtc closed this as completed Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants