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: integrate with divvy protocol #6492

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

kathaypacific
Copy link
Collaborator

@kathaypacific kathaypacific commented Feb 12, 2025

Description

This PR integrates the divvy protocol into the transaction flows:

  1. the registration transaction(s) are calculated and added to the prepared transactions the user will send so that the network fee quoted to the user can include the registration transaction
  2. the registration transactions are sent first, before the rest of the prepared transactions

Test plan

  1. the registration is completed with the user's first transaction
  2. on wallet restore, the registration is not done again
  3. the transaction flows continue to work without any user facing changes

Related issues

Backwards compatibility

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 97.72727% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.08%. Comparing base (3aa776c) to head (b26e9e6).

Files with missing lines Patch % Lines
src/divviProtocol/registerReferral.ts 96.92% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6492      +/-   ##
==========================================
+ Coverage   89.04%   89.08%   +0.04%     
==========================================
  Files         732      732              
  Lines       31847    31930      +83     
  Branches     6139     6150      +11     
==========================================
+ Hits        28358    28446      +88     
+ Misses       3442     3287     -155     
- Partials       47      197     +150     
Files with missing lines Coverage Δ
src/app/selectors.ts 96.29% <100.00%> (+0.14%) ⬆️
src/config.ts 96.39% <100.00%> (+0.06%) ⬆️
src/divviProtocol/constants.ts 100.00% <100.00%> (ø)
src/viem/index.ts 100.00% <100.00%> (ø)
src/viem/prepareTransactions.ts 99.10% <100.00%> (+0.01%) ⬆️
src/viem/saga.ts 96.49% <100.00%> (+0.65%) ⬆️
src/divviProtocol/registerReferral.ts 95.89% <96.92%> (+25.89%) ⬆️

... and 68 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3aa776c...b26e9e6. Read the comment docs.

@kathaypacific kathaypacific marked this pull request as ready for review February 12, 2025 12:35
Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Awesome! 🚀

I like that this saves complexity for builders!

Like we discussed, I'm just slightly sad that they need to know about this extra transaction if they access the returned prepared transactions. But I don't have a good alternative 🤷 🤔

@@ -22,3 +35,153 @@ export function isRegistrationTransaction(tx: TransactionRequest | SerializableT
return false
}
}

export async function createRegistrationTransactions({
Copy link
Member

Choose a reason for hiding this comment

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

nit: to make it more explicit about what it does

Suggested change
export async function createRegistrationTransactions({
export async function createRegistrationTransactionsIfNeeded({

@@ -139,6 +142,57 @@ describe('prepareTransactions module', () => {
}
const mockPublicClient = {} as unknown as jest.Mocked<(typeof publicClient)[Network.Celo]>
describe('prepareTransactions function', () => {
it('adds divvi registration transactions to the prepared transactions if there are any', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('adds divvi registration transactions to the prepared transactions if there are any', async () => {
it('adds divvi registration transactions to the prepared transactions if needed', async () => {

Comment on lines +95 to +101
nonce = yield* call(
sendPreparedRegistrationTransactions,
preparedRegistrationTransactions,
networkId,
wallet,
nonce
)
Copy link
Member

Choose a reason for hiding this comment

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

I think this shouldn't be waiting for registration TX receipt internally.
Otherwise it degrades the UX, especially on networks with longer block times.

Can we spawn the wait in sendPreparedRegistrationTransactions instead?

Comment on lines 28 to 33
const mockSendPreparedRegistrationTransactions = jest.fn()
jest.mock('src/divviProtocol/registerReferral', () => ({
...jest.requireActual('src/divviProtocol/registerReferral'),
sendPreparedRegistrationTransactions: (...args: any[]) =>
mockSendPreparedRegistrationTransactions(...args),
}))
Copy link
Member

Choose a reason for hiding this comment

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

nit: this pattern is a bit nicer for type-safety

Suggested change
const mockSendPreparedRegistrationTransactions = jest.fn()
jest.mock('src/divviProtocol/registerReferral', () => ({
...jest.requireActual('src/divviProtocol/registerReferral'),
sendPreparedRegistrationTransactions: (...args: any[]) =>
mockSendPreparedRegistrationTransactions(...args),
}))
import { sendPreparedRegistrationTransactions } from 'src/divviProtocol/registerReferral'
jest.mock('src/divviProtocol/registerReferral', () => ({
...jest.requireActual('src/divviProtocol/registerReferral'),
sendPreparedRegistrationTransactions: jest.fn()
}))
const mockSendPreparedRegistrationTransactions = jest.mocked(sendPreparedRegistrationTransactions)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i actually did try this pattern before and lost a bunch of time trying to fix the the typescript error Argument of type 'number' is not assignable to parameter of type 'never'. when i try to do mockSendPreparedRegistrationTransactions.mockResolvedValue(11). and now i realise, it's a generator function and i don't need to mock it like this at all 🤦🏻‍♀️ i've updated this test to provide mocks via .provide

Base automatically changed from kathy/fl-integration-2 to main February 12, 2025 16:31
Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

Already approved with a suggestion.

Comment on lines +154 to +158
Logger.error(
`${TAG}/sendPreparedRegistrationTransactions`,
`Failed to send or parse prepared registration transaction`,
error
)
Copy link
Member

Choose a reason for hiding this comment

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

maybe we wanna let this bubble up?
so we don't send the other transactions if we're not able to submit the TX for registering?
reducing the chance to have unattributed transactions

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

Successfully merging this pull request may close these issues.

2 participants