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.

Sign up for GitHub

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

Address Account Manager Race Condition #2102

Closed
wants to merge 11 commits into from
+236 −117
Closed

Address Account Manager Race Condition #2102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5bf99b1
Fix race condition in AccountManager
jribbink Feb 3, 2025
a9b050e
Merge remote-tracking branch 'origin/feature/cross-vm' into jribbink/…
jribbink Feb 5, 2025
82eacb0
fix tests
jribbink Feb 5, 2025
ec86613
fix merge
jribbink Feb 5, 2025
6e518cb
fix merge
jribbink Feb 5, 2025
b944c37
fix merge
jribbink Feb 5, 2025
090f682
fix tests
jribbink Feb 5, 2025
a2a821d
remove subscribe function
jribbink Feb 5, 2025
6a2bf22
fix tests
jribbink Feb 5, 2025
c6baf26
Merge remote-tracking branch 'origin/feature/cross-vm' into jribbink/…
jribbink Feb 5, 2025
dcbb13f
Fix tests
jribbink Feb 5, 2025
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
116 changes: 70 additions & 46 deletions packages/fcl-ethereum-provider/src/accounts/account-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import {mockUser} from "../__mocks__/fcl"
import * as fcl from "@onflow/fcl"
import * as rlp from "@onflow/rlp"
import {CurrentUser} from "@onflow/typedefs"
import {ChainIdStore, NetworkManager} from "../network/network-manager"
import {BehaviorSubject, Subject} from "../util/observable"

jest.mock("@onflow/fcl", () => {
const fcl = jest.requireActual("@onflow/fcl")
Expand All @@ -24,35 +26,47 @@ const mockFcl = jest.mocked(fcl)
const mockQuery = jest.mocked(fcl.query)

describe("AccountManager", () => {
let networkManager: jest.Mocked<NetworkManager>
let $mockChainId: Subject<ChainIdStore>
let userMock: ReturnType<typeof mockUser>

beforeEach(() => {
jest.clearAllMocks()

const chainId$ = new BehaviorSubject<ChainIdStore>({
chainId: 747,
error: null,
isLoading: false,
})
networkManager = {
$chainId: chainId$,
getChainId: chainId$.getValue(),
} as any as jest.Mocked<NetworkManager>
userMock = mockUser()
$mockChainId = chainId$
})

it("should initialize with null COA address", async () => {
const user = mockUser()
const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)
expect(await accountManager.getCOAAddress()).toBeNull()
expect(await accountManager.getAccounts()).toEqual([])
})

it("should reset state when the user is not logged in", async () => {
const user = mockUser()

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

expect(await accountManager.getCOAAddress()).toBeNull()
expect(await accountManager.getAccounts()).toEqual([])
})

it("should fetch and update COA address when user logs in", async () => {
const user = mockUser()
mockQuery.mockResolvedValue("0x123")

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

expect(await accountManager.getCOAAddress()).toBe(null)

user.set!({addr: "0x1"} as CurrentUser)
userMock.set!({addr: "0x1"} as CurrentUser)

expect(await accountManager.getCOAAddress()).toBe("0x123")
expect(await accountManager.getAccounts()).toEqual(["0x123"])
Expand All @@ -63,26 +77,24 @@ describe("AccountManager", () => {
})

it("should not update COA address if user has not changed", async () => {
const user = mockUser()
mockQuery.mockResolvedValue("0x123")

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

user.set!({addr: "0x1"} as CurrentUser)
userMock.set!({addr: "0x1"} as CurrentUser)

await new Promise(setImmediate)

expect(await accountManager.getCOAAddress()).toBe("0x123")
expect(fcl.query).toHaveBeenCalledTimes(1)

user.set!({addr: "0x1"} as CurrentUser)
userMock.set!({addr: "0x1"} as CurrentUser)

expect(await accountManager.getCOAAddress()).toBe("0x123")
expect(fcl.query).toHaveBeenCalledTimes(1) // Should not have fetched again
})

it("should not update COA address if fetch is outdated when user changes", async () => {
const user = mockUser()
mockQuery.mockResolvedValue("0x123")

mockQuery
Expand All @@ -93,39 +105,36 @@ describe("AccountManager", () => {
// 2nd fetch: immediate
.mockResolvedValueOnce("0x456")

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

await user.set!({addr: "0x1"} as CurrentUser)
await user.set!({addr: "0x2"} as CurrentUser)
await userMock.set!({addr: "0x1"} as CurrentUser)
await userMock.set!({addr: "0x2"} as CurrentUser)

// The second fetch (for address 0x2) is the latest, so "0x456"
expect(await accountManager.getCOAAddress()).toBe("0x456")
})

it("should throw if COA address fetch fails", async () => {
const user = mockUser()
mockQuery.mockRejectedValueOnce(new Error("Fetch failed"))

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

await user.set!({addr: "0x1"} as CurrentUser)
await userMock.set!({addr: "0x1"} as CurrentUser)

await expect(accountManager.getCOAAddress()).rejects.toThrow("Fetch failed")
})

it("should handle user changes correctly", async () => {
const user = mockUser()

mockQuery
.mockResolvedValueOnce("0x123") // for user 0x1
.mockResolvedValueOnce("0x456") // for user 0x2

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

await user.set({addr: "0x1"} as CurrentUser)
await userMock.set({addr: "0x1"} as CurrentUser)
expect(await accountManager.getCOAAddress()).toBe("0x123")

await user.set({addr: "0x2"} as CurrentUser)
await userMock.set({addr: "0x2"} as CurrentUser)

await new Promise(setImmediate)
expect(await accountManager.getCOAAddress()).toBe("0x456")
Expand All @@ -134,14 +143,12 @@ describe("AccountManager", () => {
it("should call the callback with updated accounts in subscribe", async () => {
mockQuery.mockResolvedValue("0x123")

const user = mockUser()

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

const callback = jest.fn()
accountManager.subscribe(callback)

user.set({addr: "0x1"} as CurrentUser)
userMock.set({addr: "0x1"} as CurrentUser)

await new Promise(setImmediate)

Expand All @@ -150,11 +157,10 @@ describe("AccountManager", () => {

it("should reset accounts in subscribe if user is not authenticated", async () => {
mockQuery.mockResolvedValue("0x123")
const user = mockUser()

const callback = jest.fn()

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

accountManager.subscribe(callback)

Expand All @@ -166,11 +172,11 @@ describe("AccountManager", () => {
it("should call the callback when COA address is updated", async () => {
const callback = jest.fn()

const user = mockUser({addr: "0x1"} as CurrentUser)

mockQuery.mockResolvedValueOnce("0x123")

const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(userMock.mock, networkManager)

userMock.set({addr: "0x1"} as CurrentUser)

accountManager.subscribe(callback)

Expand All @@ -180,23 +186,34 @@ describe("AccountManager", () => {
})

it("should return an empty array when COA address is null", async () => {
const {mock: user} = mockUser()
const accountManager = new AccountManager(user)
const accountManager = new AccountManager(userMock.mock, networkManager)
expect(await accountManager.getAccounts()).toEqual([])
})

it("should return COA address array when available", async () => {
mockQuery.mockResolvedValueOnce("0x123")
const {mock: user} = mockUser({addr: "0x1"} as CurrentUser)
userMock.set({addr: "0x1"} as CurrentUser)

const accountManager = new AccountManager(user)
const accountManager = new AccountManager(userMock.mock, networkManager)

expect(await accountManager.getAccounts()).toEqual(["0x123"])
})
})

describe("send transaction", () => {
let networkManager: jest.Mocked<NetworkManager>
let $mockChainId: Subject<ChainIdStore>

beforeEach(() => {
$mockChainId = new BehaviorSubject<ChainIdStore>({
chainId: 747,
error: null,
isLoading: false,
})
networkManager = {
$chainId: $mockChainId,
} as any as jest.Mocked<NetworkManager>

jest.clearAllMocks()
})

Expand All @@ -219,7 +236,7 @@ describe("send transaction", () => {
jest.mocked(fcl.query).mockResolvedValue("0x1234")

const user = mockUser({addr: "0x4444"} as CurrentUser).mock
const accountManager = new AccountManager(user)
const accountManager = new AccountManager(user, networkManager)

const tx = {
to: "0x1234",
Expand Down Expand Up @@ -266,7 +283,7 @@ describe("send transaction", () => {
jest.mocked(fcl.query).mockResolvedValue("0x1234")

const user = mockUser({addr: "0x4444"} as CurrentUser)
const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(user.mock, networkManager)

const tx = {
to: "0x1234",
Expand Down Expand Up @@ -306,7 +323,7 @@ describe("send transaction", () => {
jest.mocked(fcl.query).mockResolvedValue("0x1234")

const user = mockUser({addr: "0x4444"} as CurrentUser)
const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(user.mock, networkManager)

const tx = {
to: "0x1234",
Expand All @@ -326,7 +343,7 @@ describe("send transaction", () => {
test("throws error if from address does not match user address", async () => {
jest.mocked(fcl.query).mockResolvedValue("0x1234")
const user = mockUser({addr: "0x4444"} as CurrentUser)
const accountManager = new AccountManager(user.mock)
const accountManager = new AccountManager(user.mock, networkManager)

const tx = {
to: "0x1234",
Expand All @@ -347,6 +364,7 @@ describe("send transaction", () => {
})

describe("signMessage", () => {
let networkManager: jest.Mocked<NetworkManager>
let accountManager: AccountManager
let user: ReturnType<typeof mockUser>["mock"]
let updateUser: ReturnType<typeof mockUser>["set"]
Expand All @@ -355,7 +373,16 @@ describe("signMessage", () => {
jest.clearAllMocks()
;({mock: user, set: updateUser} = mockUser({addr: "0x123"} as CurrentUser))
jest.mocked(fcl.query).mockResolvedValue("0xCOA1")
accountManager = new AccountManager(user)
const $mockChainId = new BehaviorSubject<ChainIdStore>({
chainId: 747,
error: null,
isLoading: false,
})
networkManager = {
$chainId: $mockChainId,
getChainId: $mockChainId.getValue(),
} as any as jest.Mocked<NetworkManager>
accountManager = new AccountManager(user, networkManager)
})

it("should throw an error if the COA address is not available", async () => {
Expand Down Expand Up @@ -400,10 +427,7 @@ describe("signMessage", () => {
})

it("should throw an error if signUserMessage returns an empty array", async () => {
accountManager["coaAddress"] = "0xCOA1"

user.signUserMessage = jest.fn().mockResolvedValue([])

user.signUserMessage.mockResolvedValue([])
await expect(
accountManager.signMessage("Test message", "0xCOA1")
).rejects.toThrow("Failed to sign message")
Expand Down
Loading