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

feat(api-kit): Support for SafeOperation confirmations endpoints #876

Merged
merged 15 commits into from
Jul 1, 2024
Merged
Show file tree
Hide file tree
Changes from 8 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
70 changes: 66 additions & 4 deletions packages/api-kit/src/SafeApiKit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
AllTransactionsOptions,
DeleteSafeDelegateProps,
GetSafeDelegateProps,
GetSafeMessageListProps,
GetSafeOperationConfirmationListProps,
GetSafeOperationListProps,
GetSafeOperationListResponse,
SafeSingletonResponse,
GetSafeMessageListProps,
ModulesResponse,
OwnerResponse,
ProposeTransactionProps,
Expand All @@ -24,6 +24,7 @@
SafeMultisigTransactionEstimateResponse,
SafeMultisigTransactionListResponse,
SafeServiceInfoResponse,
SafeSingletonResponse,
SignatureResponse,
TokenInfoListResponse,
TokenInfoResponse,
Expand All @@ -34,11 +35,12 @@
import { validateEip3770Address, validateEthereumAddress } from '@safe-global/protocol-kit'
import {
Eip3770Address,
isSafeOperation,
SafeMultisigConfirmationListResponse,
SafeMultisigTransactionResponse,
SafeOperationResponse,
SafeOperation,
isSafeOperation
SafeOperationConfirmationListResponse,
SafeOperationResponse
} from '@safe-global/safe-core-sdk-types'
import { TRANSACTION_SERVICE_URLS } from './utils/config'
import { isEmptyData } from './utils'
Expand Down Expand Up @@ -118,7 +120,7 @@
* @throws "Not Found"
* @throws "Ensure this field has at least 1 hexadecimal chars (not counting 0x)."
*/
async decodeData(data: string): Promise<any> {

Check warning on line 123 in packages/api-kit/src/SafeApiKit.ts

View workflow job for this annotation

GitHub Actions / eslint

Unexpected any. Specify a different type
if (data === '') {
throw new Error('Invalid data')
}
Expand Down Expand Up @@ -328,7 +330,7 @@
const { address: delegator } = this.#getEip3770Address(delegatorAddress)
const signature = await signDelegate(signer, delegate, this.#chainId)

const body: any = {

Check warning on line 333 in packages/api-kit/src/SafeApiKit.ts

View workflow job for this annotation

GitHub Actions / eslint

Unexpected any. Specify a different type
safe: safeAddress ? this.#getEip3770Address(safeAddress).address : null,
delegate,
delegator,
Expand Down Expand Up @@ -852,6 +854,66 @@
}
})
}

/**
* Returns the list of confirmations for a given a SafeOperation.
*
* @param getSafeOperationConfirmationsProps - The parameters for fetching the list of confirmations
* @returns The list of confirmations
* @throws "Invalid SafeOperation hash"
* @throws "Invalid data"
*/
async getSafeOperationConfirmations({
Copy link
Contributor

Choose a reason for hiding this comment

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

Please double check how we do this. This kind of looks like a getSafeOperationConfirmationsByHash with a different name as the hash itself is a part of a "perma-link". In this case, it would be common to separate the interface as getSafeOperationConfirmations(hash, { limit, offset }) since that would be somehow associated with a domain model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the function signature accordingly 👍

safeOperationHash,
limit,
offset
}: GetSafeOperationConfirmationListProps): Promise<SafeOperationConfirmationListResponse> {
if (!safeOperationHash) {
throw new Error('Invalid SafeOperation hash')
}

const url = new URL(
`${this.#txServiceBaseUrl}/v1/safe-operations/${safeOperationHash}/confirmations/`
)

if (limit) {
url.searchParams.set('limit', limit)
}

if (offset) {
url.searchParams.set('offset', offset)
}

return sendRequest({
url: url.toString(),
method: HttpMethod.Get
})
}

/**
* Adds a confirmation for a SafeOperation.
*
* @param safeOperationHash The SafeOperation hash
* @param signature - Signature of the SafeOperation
* @returns
* @throws "Invalid SafeOperation hash"
* @throws "Invalid signature"
* @throws "Malformed data"
* @throws "Error processing data"
*/
async confirmSafeOperation(safeOperationHash: string, signature: string): Promise<void> {
if (!safeOperationHash) {
throw new Error('Invalid SafeOperation hash')
}
if (!signature) {
throw new Error('Invalid signature')
}
return sendRequest({
url: `${this.#txServiceBaseUrl}/v1/safe-operations/${safeOperationHash}/confirmations/`,
method: HttpMethod.Post,
body: { signature }
})
}
}

export default SafeApiKit
9 changes: 9 additions & 0 deletions packages/api-kit/src/types/safeTransactionServiceTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -325,3 +325,12 @@ export type AddSafeOperationProps = {
validAfter?: number
}
}

export type GetSafeOperationConfirmationListProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a little bit weird to use limit and offset as a string since they are associated with record count in a database. I imagine that the interface will be something like apiKit.getSafeOperationConfirmations({ safeOperationHash, limit: '10' }).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes good point. Changed the type to number

/** The hash of the SafeOperation to get confirmations for */
safeOperationHash: string
/** Maximum number of results to return per page */
limit?: string
/** Initial index from which to return the results */
offset?: string
}
6 changes: 3 additions & 3 deletions packages/api-kit/tests/e2e/addSafeOperation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import { ethers } from 'ethers'
import sinon from 'sinon'
import sinonChai from 'sinon-chai'
import Safe from '@safe-global/protocol-kit'
import SafeApiKit from '@safe-global/api-kit'
import SafeApiKit from '@safe-global/api-kit/index'
import { Safe4337Pack } from '@safe-global/relay-kit'
import { generateTransferCallData } from '@safe-global/relay-kit/src/packs/safe-4337/testing-utils/helpers'
import { generateTransferCallData } from '@safe-global/relay-kit/packs/safe-4337/testing-utils/helpers'
import { RPC_4337_CALLS } from '@safe-global/relay-kit/packs/safe-4337/constants'
import { getKits } from '../utils/setupKits'
import { getAddSafeOperationProps } from '@safe-global/api-kit/utils/safeOperation'
Expand All @@ -15,7 +15,7 @@ chai.use(chaiAsPromised)
chai.use(sinonChai)

const SIGNER_PK = '0x83a415ca62e11f5fa5567e98450d0f82ae19ff36ef876c10a8d448c788a53676'
const SAFE_ADDRESS = '0x60C4Ab82D06Fd7dFE9517e17736C2Dcc77443EF0' // 1/1 Safe (v1.4.1) with signer above as owner + 4337 module enabled
const SAFE_ADDRESS = '0x60C4Ab82D06Fd7dFE9517e17736C2Dcc77443EF0' // 1/2 Safe (v1.4.1) with signer above being an owner + 4337 module enabled
const PAYMASTER_TOKEN_ADDRESS = '0x1c7D4B196Cb0C7B01d743Fbc6116a902379C7238'
const PAYMASTER_ADDRESS = '0x0000000000325602a77416A16136FDafd04b299f'
const BUNDLER_URL = `https://bundler.url`
Expand Down
114 changes: 114 additions & 0 deletions packages/api-kit/tests/e2e/confirmSafeOperation.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import chai from 'chai'
import chaiAsPromised from 'chai-as-promised'
import { Safe4337InitOptions, Safe4337Pack } from '@safe-global/relay-kit'
import { generateTransferCallData } from '@safe-global/relay-kit/packs/safe-4337/testing-utils/helpers'
import SafeApiKit from '@safe-global/api-kit/index'
import { getAddSafeOperationProps } from '@safe-global/api-kit/utils/safeOperation'
import { SafeOperation } from '@safe-global/safe-core-sdk-types'
import { getApiKit, getEip1193Provider } from '../utils/setupKits'

chai.use(chaiAsPromised)

const PRIVATE_KEY_1 = '0x83a415ca62e11f5fa5567e98450d0f82ae19ff36ef876c10a8d448c788a53676'
const PRIVATE_KEY_2 = '0xb88ad5789871315d0dab6fc5961d6714f24f35a6393f13a6f426dfecfc00ab44'
const SAFE_ADDRESS = '0x60C4Ab82D06Fd7dFE9517e17736C2Dcc77443EF0' // 4337 enabled 1/2 Safe (v1.4.1) owned by PRIVATE_KEY_1 + PRIVATE_KEY_2
const TX_SERVICE_URL = 'https://safe-transaction-sepolia.staging.5afe.dev/api'
const BUNDLER_URL = `https://bundler.url`
const PAYMASTER_TOKEN_ADDRESS = '0x1c7D4B196Cb0C7B01d743Fbc6116a902379C7238'

let safeApiKit: SafeApiKit
let safe4337Pack: Safe4337Pack
let safeOperation: SafeOperation
let safeOpHash: string

describe('confirmSafeOperation', () => {
const transferUSDC = {
to: PAYMASTER_TOKEN_ADDRESS,
data: generateTransferCallData(SAFE_ADDRESS, 100_000n),
value: '0',
operation: 0
}

const getSafe4337Pack = async (options: Partial<Safe4337InitOptions>) =>
Safe4337Pack.init({
provider: options.provider || getEip1193Provider(),
signer: options.signer || PRIVATE_KEY_1,
options: { safeAddress: SAFE_ADDRESS },
bundlerUrl: BUNDLER_URL
})

const createSignature = async (safeOperation: SafeOperation, signer: string) => {
const safe4337Pack = await getSafe4337Pack({ signer })
const signedSafeOperation = await safe4337Pack.signSafeOperation(safeOperation)
const signerAddress = await safe4337Pack.protocolKit.getSafeProvider().getSignerAddress()
return signedSafeOperation.getSignature(signerAddress!)
}

/**
* Add a new Safe operation to the transaction service.
* @returns Resolves with the signed Safe operation
*/
const addSafeOperation = async (): Promise<SafeOperation> => {
const safeOperation = await safe4337Pack.createTransaction({
transactions: [transferUSDC]
})

const signedSafeOperation = await safe4337Pack.signSafeOperation(safeOperation)
const addSafeOperationProps = await getAddSafeOperationProps(signedSafeOperation)

await safeApiKit.addSafeOperation(addSafeOperationProps)

return signedSafeOperation
}

before(async () => {
safe4337Pack = await getSafe4337Pack({ signer: PRIVATE_KEY_1 })
safeApiKit = getApiKit(TX_SERVICE_URL)

// Submit a new Safe operation to the transaction service
safeOperation = await addSafeOperation()
safeOpHash = await safe4337Pack.getSafeOperationHash(safeOperation)
})

describe('should fail', () => {
it('if SafeOperation hash is empty', async () => {
const signature = await createSignature(safeOperation, PRIVATE_KEY_2)
await chai
.expect(safeApiKit.confirmSafeOperation('', signature!.data))
.to.be.rejectedWith('Invalid SafeOperation hash')
})

it('if signature is empty', async () => {
await chai
.expect(safeApiKit.confirmSafeOperation(safeOpHash, ''))
.to.be.rejectedWith('Invalid signature')
})

it('if signature is invalid', async () => {
await chai
.expect(safeApiKit.confirmSafeOperation(safeOpHash, '0xInvalidSignature'))
.to.be.rejectedWith('Bad Request')
})
})

it('should allow to create and confirm a SafeOperation signature using a Safe signer', async () => {
const signerAddress1 = await safe4337Pack.protocolKit.getSafeProvider().getSignerAddress()

// Create a signature for the Safe operation using owner 2
const signatureSigner2 = await createSignature(safeOperation, PRIVATE_KEY_2)

// Add the second signature to the Safe operation
await chai.expect(safeApiKit.confirmSafeOperation(safeOpHash, signatureSigner2!.data)).to.be
.fulfilled

// Check that the Safe operation is now confirmed by both owners
const safeOperationResponse = await safeApiKit.getSafeOperation(safeOpHash)
chai.expect(safeOperationResponse.confirmations).to.have.lengthOf(2)

chai
.expect(safeOperationResponse.confirmations![0].signature)
.to.eq(safeOperation.getSignature(signerAddress1!)!.data)

chai.expect(safeOperationResponse.confirmations![1].signature).to.eq(signatureSigner2!.data)
})
})
2 changes: 1 addition & 1 deletion packages/api-kit/tests/e2e/getSafeOperation.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import SafeApiKit from '@safe-global/api-kit'
import SafeApiKit from '@safe-global/api-kit/index'
import chai from 'chai'
import chaiAsPromised from 'chai-as-promised'
import { getApiKit } from '../utils/setupKits'
Expand Down
40 changes: 40 additions & 0 deletions packages/api-kit/tests/e2e/getSafeOperationConfirmations.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import chai from 'chai'
import chaiAsPromised from 'chai-as-promised'
import SafeApiKit from '@safe-global/api-kit/index'
import { getApiKit } from '../utils/setupKits'

chai.use(chaiAsPromised)

let safeApiKit: SafeApiKit

const TX_SERVICE_URL = 'https://safe-transaction-sepolia.staging.5afe.dev/api'

describe('getSafeOperationConfirmations', () => {
before(async () => {
safeApiKit = getApiKit(TX_SERVICE_URL)
})

it('should fail if safeOperationHash is empty', async () => {
await chai
.expect(safeApiKit.getSafeOperationConfirmations({ safeOperationHash: '' }))
.to.be.rejectedWith('Invalid SafeOperation hash')
})

it('should return an empty array if the safeOperationHash is not found', async () => {
const safeOperationHash = '0x0000000000000000000000000000000000000000000000000000000000000000'
const safeOpConfirmations = await safeApiKit.getSafeOperationConfirmations({
safeOperationHash
})
chai.expect(safeOpConfirmations.count).to.be.equal(0)
chai.expect(safeOpConfirmations.results.length).to.be.equal(0)
})

it('should return the SafeOperation with the given safeOperationHash', async () => {
const safeOperationHash = '0x375d3bd580600ce04d7d2c1d8d88d85f27b9c7d14d7b544f2ee585d672f2b449'
const safeOpConfirmations = await safeApiKit.getSafeOperationConfirmations({
safeOperationHash
})
chai.expect(safeOpConfirmations.count).to.be.equal(2)
chai.expect(safeOpConfirmations.results.length).to.be.equal(2)
})
})
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import SafeApiKit from '@safe-global/api-kit'
import SafeApiKit from '@safe-global/api-kit/index'
import chai from 'chai'
yagopv marked this conversation as resolved.
Show resolved Hide resolved
import chaiAsPromised from 'chai-as-promised'
import { getApiKit } from '../utils/setupKits'
Expand Down
39 changes: 34 additions & 5 deletions packages/api-kit/tests/endpoint/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const eip3770DelegateAddress = `${config.EIP_3770_PREFIX}:${delegateAddress}`
const tokenAddress = '0xfFf9976782d46CC05630D1f6eBAb18b2324d6B14'
const eip3770TokenAddress = `${config.EIP_3770_PREFIX}:${tokenAddress}`
const safeTxHash = '0x317834aea988fd3cfa54fd8b2be2c96b4fd70a14d8c9470a7110576b01e6480a'
const safeOpHash = '0x8b1840745ec0a6288e868c6e285dadcfebd49e846d307610a9ccd97f445ace93'
const txServiceBaseUrl = 'https://safe-transaction-sepolia.safe.global/api'
const defaultProvider = getDefaultProvider(config.JSON_RPC)
const signer = new Wallet(PRIVATE_KEY_1, defaultProvider)
Expand All @@ -51,6 +52,10 @@ describe('Endpoint tests', () => {
.stub(httpRequests, 'sendRequest')
.returns(Promise.resolve({ data: { success: true } }))

afterEach(() => {
sinon.resetHistory()
})

describe('Default txServiceUrl', () => {
it('getServiceInfo', async () => {
await chai
Expand Down Expand Up @@ -151,7 +156,8 @@ describe('Endpoint tests', () => {
.to.be.eventually.deep.equals({ data: { success: true } })
chai.expect(fetchData).to.have.been.calledWith({
url: `${txServiceBaseUrl}/v1/multisig-transactions/${safeTxHash}/confirmations/`,
method: 'get'
method: 'post',
body: { signature: '0x' }
dasanra marked this conversation as resolved.
Show resolved Hide resolved
})
})

Expand Down Expand Up @@ -649,13 +655,11 @@ describe('Endpoint tests', () => {
})

it('getSafeOperation', async () => {
const safeOperationHash = 'safe-operation-hash'

await chai
.expect(safeApiKit.getSafeOperation(safeOperationHash))
.expect(safeApiKit.getSafeOperation(safeOpHash))
.to.be.eventually.deep.equals({ data: { success: true } })
chai.expect(fetchData).to.have.been.calledWith({
url: `${txServiceBaseUrl}/v1/safe-operations/${safeOperationHash}/`,
url: `${txServiceBaseUrl}/v1/safe-operations/${safeOpHash}/`,
method: 'get'
})
})
Expand Down Expand Up @@ -713,6 +717,31 @@ describe('Endpoint tests', () => {
}
})
})

it('getSafeOperationConfirmations', async () => {
await chai
.expect(safeApiKit.getSafeOperationConfirmations({ safeOperationHash: safeOpHash }))
.to.be.eventually.deep.equals({ data: { success: true } })
chai.expect(fetchData).to.have.been.calledWith({
url: `${txServiceBaseUrl}/v1/safe-operations/${safeOpHash}/confirmations/`,
method: 'get'
})
})

it('confirmSafeOperation', async () => {
const senderSignature = await protocolKit.signHash(safeOpHash)
await chai
.expect(safeApiKit.confirmSafeOperation(safeOpHash, senderSignature.data))
.to.be.eventually.deep.equals({ data: { success: true } })
chai.expect(fetchData).to.have.been.calledWith({
url: `${txServiceBaseUrl}/v1/safe-operations/${safeOpHash}/confirmations/`,
method: 'post',
body: {
signature:
'0x23e70757bcfe1ccba40588ee3ec8fbca7ff4b51ad48703a598fbe98b935b927d064a98cdcfaf2eff00ae05628ed38f15799a732e143f9fce7215a62fe8e14ff020'
}
})
})
})

describe('Custom endpoint', () => {
Expand Down
Loading
Loading