Skip to content

Commit

Permalink
fix: owner management transaction
Browse files Browse the repository at this point in the history
  • Loading branch information
@iamacook
iamacook committed Nov 15, 2023
1 parent 76ee0df commit 6b338f2
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 38 deletions.
94 changes: 70 additions & 24 deletions src/services/recovery/__tests__/transaction.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ describe('transaction', () => {
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [oldOwner2, oldOwner3, newOwner3])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [newOwner2, oldOwner3, newOwner3])
})
})

Expand Down Expand Up @@ -115,8 +115,8 @@ describe('transaction', () => {
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [oldOwner2, oldOwner3, newOwner3])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [newOwner2, oldOwner3, newOwner3])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(4, 'changeThreshold', [newThreshold])
})
})
Expand Down Expand Up @@ -176,7 +176,7 @@ describe('transaction', () => {
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2])
})
})

Expand Down Expand Up @@ -211,7 +211,7 @@ describe('transaction', () => {
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'changeThreshold', [newThreshold])
})
})
Expand Down Expand Up @@ -309,7 +309,7 @@ describe('transaction', () => {
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, oldThreshold])
})
})
Expand Down Expand Up @@ -345,7 +345,7 @@ describe('transaction', () => {
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, newThreshold])
})
})
Expand Down Expand Up @@ -374,36 +374,58 @@ describe('transaction', () => {
expect(transactions).toHaveLength(2)

expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2)
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner2, 1])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner2, oldThreshold])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner3, oldThreshold])
})

it('should swap as many differing owners as possible then add the rest when with a final threshold change if the threshold changes', () => {
const safeAddresss = faker.finance.ethereumAddress()

const oldOwner1 = faker.finance.ethereumAddress()

const newOwner1 = faker.finance.ethereumAddress()
const newOwner2 = faker.finance.ethereumAddress()
const newOwner3 = faker.finance.ethereumAddress()
const newOwner4 = faker.finance.ethereumAddress()
const newOwner5 = faker.finance.ethereumAddress()

const oldThreshold = 2
const newThreshold = oldThreshold + 1
const oldOwner1 = faker.finance.ethereumAddress()

const oldThreshold = 1
const newThreshold = 4

const safe = {
address: { value: safeAddresss },
owners: [{ value: oldOwner1 }],
threshold: oldThreshold,
} as SafeInfo

const newOwners = [{ value: oldOwner1 }, { value: newOwner1 }, { value: newOwner2 }]
const newOwners = [
{ value: newOwner1 },
{ value: newOwner2 },
{ value: newOwner3 },
{ value: newOwner4 },
{ value: newOwner5 },
]

const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners })

expect(transactions).toHaveLength(2)
expect(transactions).toHaveLength(5)

expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2)
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner1, 1])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner2, newThreshold])
expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(5)
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [
SENTINEL_ADDRESS,
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [
newOwner2,
2, // Intemediary threshold - length of current owners
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [
newOwner3,
3, // Intemediary threshold - length of current owners
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(4, 'addOwnerWithThreshold', [newOwner4, newThreshold])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(5, 'addOwnerWithThreshold', [newOwner5, newThreshold])
})
})
})
Expand Down Expand Up @@ -444,8 +466,8 @@ describe('transaction', () => {
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [oldOwner2, oldOwner3, oldThreshold])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [newOwner2, oldOwner3, oldThreshold])
})

it('should swap as many owners as possible then remove the rest when with a final threshold change if the threshold changes', () => {
Expand Down Expand Up @@ -479,8 +501,8 @@ describe('transaction', () => {
oldOwner1,
newOwner1,
])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [oldOwner2, oldOwner3, newThreshold])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [newOwner2, oldOwner3, newThreshold])
})
})

Expand Down Expand Up @@ -510,7 +532,7 @@ describe('transaction', () => {

expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2)
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [oldOwner1, oldOwner2, newOwner1])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [oldOwner2, oldOwner3, oldThreshold])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [newOwner1, oldOwner3, oldThreshold])
})

it('should swap as many differing owners as possible then remove the rest when with a final threshold change if the threshold changes', () => {
Expand Down Expand Up @@ -539,10 +561,34 @@ describe('transaction', () => {

expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2)
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [oldOwner1, oldOwner2, newOwner1])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [oldOwner2, oldOwner3, newThreshold])
expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [newOwner1, oldOwner3, newThreshold])
})
})
})

it('should throw if the new threshold is higher than the final owner output', () => {
const safeAddresss = faker.finance.ethereumAddress()

const newOwner1 = faker.finance.ethereumAddress()
const newOwner2 = faker.finance.ethereumAddress()

const oldOwner1 = faker.finance.ethereumAddress()

const oldThreshold = 1
const newThreshold = 10

const safe = {
address: { value: safeAddresss },
owners: [{ value: oldOwner1 }],
threshold: oldThreshold,
} as SafeInfo

const newOwners = [{ value: newOwner1 }, { value: newOwner2 }]

expect(() => getRecoveryProposalTransactions({ safe, newThreshold, newOwners })).toThrow(
'New threshold is higher than desired owners',
)
})
})

describe('getRecoveryProposalTransaction', () => {
Expand Down
56 changes: 42 additions & 14 deletions src/services/recovery/transaction.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ export function getRecoveryProposalTransactions({
newThreshold: number
newOwners: Array<AddressEx>
}): Array<MetaTransactionData> {
const INTERMEDIARY_THRESHOLD = 1

const safeDeployment = getSafeSingletonDeployment({ network: safe.chainId, version: safe.version ?? undefined })

if (!safeDeployment) {
Expand All @@ -40,40 +38,70 @@ export function getRecoveryProposalTransactions({
// Check whether threshold should be changed after owner management
let changeThreshold = newThreshold !== safe.threshold

// Owner management transaction data
const txData: Array<string> = []

// Minimum number of owner management transactions required
const length = Math.max(ownersToAdd.length, ownersToRemove.length)

// Iterate of existing/new owners and swap, add, remove accordingly
for (let index = 0; index < length; index++) {
const ownerToAdd = ownersToAdd[index]?.value
const ownerToRemove = ownersToRemove[index]?.value

const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove))
const prevOwner = ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1].value
const prevOwner = (() => {
const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove))
return ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1]?.value
})()

// Swap owner if possible
// Swap existing owner with new one
if (ownerToRemove && ownerToAdd) {
txData.push(safeInterface.encodeFunctionData('swapOwner', [prevOwner, ownerToRemove, ownerToAdd]))
const swapOwner = safeInterface.encodeFunctionData('swapOwner', [prevOwner, ownerToRemove, ownerToAdd])
txData.push(swapOwner)

// Swap owner in cache
_owners = _owners.map((owner) => (sameAddress(owner.value, ownerToRemove) ? ownersToAdd[index] : owner))

continue
}

// Add or remove owner, finally setting threshold (intermediary value prevents threshold > owner length)
const threshold = index === length - 1 ? newThreshold : INTERMEDIARY_THRESHOLD
const threshold = (() => {
const newOwnerLength = ownerToAdd ? _owners.length + 1 : _owners.length - 1

// Final transaction
if (index === length - 1) {
if (newThreshold > newOwnerLength) {
throw new Error('New threshold is higher than desired owners')
}
return newThreshold
}

// Prevent intermediary threshold > number of owners
return newThreshold > newOwnerLength ? newOwnerLength : newThreshold
})()

// Add new owner and set threshold
if (ownerToAdd) {
const addOwnerWithThreshold = safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold])
txData.push(addOwnerWithThreshold)

// Add owner to cache
_owners.push({ value: ownerToAdd })
}
// Remove existing owner and set threshold
else {
const removeOwner = safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold])
txData.push(removeOwner)

if (!ownerToRemove) {
txData.push(safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold]))
_owners.push(ownersToAdd[index])
} else {
txData.push(safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold]))
// Remove owner from cache
_owners = _owners.filter((owner) => !sameAddress(owner.value, ownerToRemove))
}

// addOwnerWithThreshold/removeOwner changed threshold
changeThreshold = false
}

// If only swapOwners exist but there is a threshold change, changeThreshold
// Only swapOwner will be called
if (changeThreshold) {
txData.push(safeInterface.encodeFunctionData('changeThreshold', [newThreshold]))
}
Expand Down

0 comments on commit 6b338f2

Please sign in to comment.