Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion yarn-project/cli-wallet/src/utils/wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ export class CLIWallet extends BaseWallet {
opts: SimulateViaEntrypointOptions,
): Promise<TxSimulationResultWithAppOffset> {
const { from, feeOptions, additionalScopes, sendMessagesAs } = opts;
const scopes = this.scopesFrom(from, additionalScopes);
const scopes = this.scopesFrom(from, additionalScopes, sendMessagesAs);
const feeExecutionPayload = await feeOptions.walletFeePaymentMethod?.getExecutionPayload();
const finalExecutionPayload = feeExecutionPayload
? mergeExecutionPayloads([feeExecutionPayload, executionPayload])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,19 @@ describe('PrivateExecutionOracle', () => {
});
});

describe('getAppTaggingSecret', () => {
it('rejects a sender outside the execution scopes', async () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably add a test that actually doesn't throw when the sender is scoped correctly?

const inScopeSender = await AztecAddress.random();
const outOfScopeSender = await AztecAddress.random();
const recipient = await AztecAddress.random();
const oracle = makeOracle({ scopes: [inScopeSender] });

await expect(oracle.getAppTaggingSecret(outOfScopeSender, recipient)).rejects.toThrow(
'is not in the allowed scopes list',
);
});
});

describe('resolveTaggingStrategy', () => {
let sender: AztecAddress;
let recipient: AztecAddress;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
type TaggingSecretStrategy,
} from '../../hooks/resolve_tagging_secret_strategy.js';
import { NoteService } from '../../notes/note_service.js';
import { assertAllowedScope } from '../../storage/allowed_scopes.js';
import type { SenderTaggingStore } from '../../storage/tagging_store/sender_tagging_store.js';
import { syncSenderTaggingIndexes } from '../../tagging/index.js';
import type { ExecutionNoteCache } from '../execution_note_cache.js';
Expand Down Expand Up @@ -321,7 +322,17 @@ export class PrivateExecutionOracle extends UtilityExecutionOracle implements IP
* @returns The app tagging secret, or `None` if the recipient is invalid.
*/
public async getAppTaggingSecret(sender: AztecAddress, recipient: AztecAddress): Promise<Option<Fr>> {
const extendedSecret = await this.#calculateAppTaggingSecret(this.contractAddress, sender, recipient);
assertAllowedScope(sender, this.scopes);

const senderCompleteAddress = await this.getCompleteAddressOrFail(sender);
const senderIvsk = await this.keyStore.getMasterIncomingViewingSecretKey(sender);
const extendedSecret = await AppTaggingSecret.computeViaEcdh(
senderCompleteAddress,
senderIvsk,
recipient,
this.contractAddress,
recipient,
);

if (!extendedSecret) {
this.logger.warn(`Computing a tagging secret for invalid recipient ${recipient} - returning no secret`, {
Expand Down Expand Up @@ -354,12 +365,6 @@ export class PrivateExecutionOracle extends UtilityExecutionOracle implements IP
return index;
}

async #calculateAppTaggingSecret(contractAddress: AztecAddress, sender: AztecAddress, recipient: AztecAddress) {
const senderCompleteAddress = await this.getCompleteAddressOrFail(sender);
const senderIvsk = await this.keyStore.getMasterIncomingViewingSecretKey(sender);
return AppTaggingSecret.computeViaEcdh(senderCompleteAddress, senderIvsk, recipient, contractAddress, recipient);
}

async #getIndexToUseForSecret(secret: AppTaggingSecret): Promise<number> {
// If we have the tagging index in the cache, we use it. If not we obtain it from the execution data provider.
const lastUsedIndexInTx = this.taggingIndexCache.getLastUsedIndex(secret);
Expand Down
17 changes: 12 additions & 5 deletions yarn-project/wallet-sdk/src/base-wallet/base_wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,15 @@ export abstract class BaseWallet implements Wallet {
protected log = createLogger('wallet-sdk:base_wallet'),
) {}

protected scopesFrom(from: AztecAddress | NoFrom, additionalScopes: AztecAddress[] = []): AztecAddress[] {
const allScopes = from === NO_FROM ? additionalScopes : [from, ...additionalScopes];
protected scopesFrom(
from: AztecAddress | NoFrom,
additionalScopes: AztecAddress[] = [],
sendMessagesAs?: AztecAddress,
Comment on lines +130 to +131

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I really think we shouldn't use default values and optional params. I think we should force each caller to make a decision, or we could end up with another bug like the current one

Suggested change
additionalScopes: AztecAddress[] = [],
sendMessagesAs?: AztecAddress,
additionalScopes: AztecAddress[],
sendMessagesAs: AztecAddress | undefined,

): AztecAddress[] {
// The sendMessageAs account must be in scope so that its tagging secrets can be accessed.
const tagSenderScopes = sendMessagesAs ? [sendMessagesAs] : [];
const baseScopes = from === NO_FROM ? [] : [from];
const allScopes = [...baseScopes, ...additionalScopes, ...tagSenderScopes];
const scopeSet = new Set(allScopes.map(address => address.toString()));
return [...scopeSet].map(AztecAddress.fromStringUnsafe);
}
Expand Down Expand Up @@ -402,7 +409,7 @@ export abstract class BaseWallet implements Wallet {
simulatePublic: true,
skipTxValidation: opts.skipTxValidation,
skipFeeEnforcement: opts.skipFeeEnforcement,
scopes: this.scopesFrom(opts.from, opts.additionalScopes),
scopes: this.scopesFrom(opts.from, opts.additionalScopes, opts.sendMessagesAs),
senderForTags: this.senderForTagsFrom(opts.from, opts.sendMessagesAs),
overrides: opts.overrides,
});
Expand Down Expand Up @@ -498,7 +505,7 @@ export abstract class BaseWallet implements Wallet {
return this.pxe.profileTx(txRequest, {
profileMode: opts.profileMode,
skipProofGeneration: opts.skipProofGeneration ?? true,
scopes: this.scopesFrom(opts.from, opts.additionalScopes),
scopes: this.scopesFrom(opts.from, opts.additionalScopes, opts.sendMessagesAs),
senderForTags: this.senderForTagsFrom(opts.from, opts.sendMessagesAs),
});
}
Expand All @@ -515,7 +522,7 @@ export abstract class BaseWallet implements Wallet {
});
const txRequest = await this.createTxExecutionRequestFromPayloadAndFee(executionPayload, opts.from, feeOptions);
const provenTx = await this.pxe.proveTx(txRequest, {
scopes: this.scopesFrom(opts.from, opts.additionalScopes),
scopes: this.scopesFrom(opts.from, opts.additionalScopes, opts.sendMessagesAs),
senderForTags: this.senderForTagsFrom(opts.from, opts.sendMessagesAs),
});
const offchainOutput = extractOffchainOutput(
Expand Down
7 changes: 5 additions & 2 deletions yarn-project/wallets/src/embedded/embedded_wallet.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('EmbeddedWallet', () => {
});

describe('sendTx', () => {
it('passes sendMessagesAs as senderForTags to PXE simulation', async () => {
it('passes sendMessagesAs as senderForTags and includes it in scopes for PXE simulation', async () => {
getPredictedMinFees.mockResolvedValue([new GasFees(2, 2)]);
getNodeInfo.mockResolvedValue({
l1ChainId: 1,
Expand All @@ -69,7 +69,10 @@ describe('EmbeddedWallet', () => {

expect(simulateTx).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ senderForTags: sendMessagesAs }),
expect.objectContaining({
senderForTags: sendMessagesAs,
scopes: expect.arrayContaining([sendMessagesAs]),
}),
);
});

Expand Down
2 changes: 1 addition & 1 deletion yarn-project/wallets/src/embedded/embedded_wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ export class EmbeddedWallet extends BaseWallet {
opts: SimulateViaEntrypointOptions,
): Promise<TxSimulationResultWithAppOffset> {
const { from, feeOptions, additionalScopes, skipTxValidation, skipFeeEnforcement, sendMessagesAs } = opts;
const scopes = this.scopesFrom(from, additionalScopes);
const scopes = this.scopesFrom(from, additionalScopes, sendMessagesAs);

const feeExecutionPayload = await feeOptions.walletFeePaymentMethod?.getExecutionPayload();
const finalExecutionPayload = feeExecutionPayload
Expand Down
Loading