-
Notifications
You must be signed in to change notification settings - Fork 17
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(svm): svm spoke events client #899
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
@@ -12,6 +12,8 @@ | |||
"node": ">=20.18.0" | |||
}, | |||
"scripts": { | |||
"build-bigint-buffer": "[ -d node_modules/bigint-buffer ] && command -v node-gyp > /dev/null && cd node_modules/bigint-buffer && node-gyp configure && node-gyp build || echo 'Skipping bigint-buffer build: folder or node-gyp not found'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as in relayer to get rid of the bigint-buffer warning
@@ -105,7 +107,9 @@ | |||
"@eth-optimism/sdk": "^3.3.1", | |||
"@ethersproject/bignumber": "^5.7.0", | |||
"@pinata/sdk": "^2.1.0", | |||
"@solana/web3.js": "^2.0.0", | |||
"@solana/web3-v2.js": "npm:@solana/web3.js@2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to have both versions because the BorshEncoder uses v1 internally
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
src/svm/utils/events.ts
Outdated
* Gets the event name from a raw name. | ||
*/ | ||
export function getEventName(rawName?: string): EventName { | ||
if (!rawName) throw new Error("Raw name is undefined"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should never happen so we throw an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why accept optional param in the first place?
src/svm/utils/events.ts
Outdated
case "TransferredOwnership": | ||
return eventData as TransferredOwnershipEvent; | ||
default: | ||
throw new Error(`Unknown event name: ${name}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
src/svm/eventsClient.ts
Outdated
import { getEventName, parseEventData } from "./utils/events"; | ||
import { isDevnet } from "./utils/helpers"; | ||
|
||
type GetTransactionReturnType = ReturnType<GetTransactionApi["getTransaction"]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be bit risky as this infers the union of all overloads. E.g. if the caller passed { encoding: "base58" }
then returned transaction
property would be of type Base58EncodedDataResponse
and the event client would error when trying to access something like txResult.transaction.message.accountKeys
.
I think the above does not exactly apply to us as we are calling getTransaction
without setting encoding, so it matches the optional json overload which has the props we are trying to access. But maybe its worth adding a comment on this for safer code maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I've added a type wrapper to extract only the "json" encoded return type.
src/svm/eventsClient.ts
Outdated
): Promise<{ program: Address; data: EventData; name: EventName }[]> { | ||
if (!txResult) return []; | ||
|
||
const eventAuthorities: Map<string, Address> = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is mapping necessary? we only have single program in this method
src/svm/eventsClient.ts
Outdated
programId == ixProgramId && | ||
eventAuthorities.get(ixProgramId.toString()) == singleIxAccount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these use strict evaluation ===
?
src/svm/eventsClient.ts
Outdated
const name = getEventName(event?.name); | ||
events.push({ | ||
program: programId, | ||
data: parseEventData(event?.data), | ||
name, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe parsing should be in separate function that caller can try to parse. getEventName
and parseEventData
would not work if the caller passed other program than spoke pool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively, we could omit passing program from all private methods and use only the spoke pool
src/svm/eventsClient.ts
Outdated
fromSlot?: bigint, | ||
toSlot?: bigint, | ||
options: GetSignaturesForAddressConfig = { limit: 1000 }, | ||
finality: Commitment = "confirmed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we better use commitment
for the parameter name.
src/svm/eventsClient.ts
Outdated
program: Address, | ||
anchorIdl: Idl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these might be not necessary as we use them in private methods and it should work only for the spoke pool program. Except, maybe keep program address, but that is useful only if the public create
method allowed passing a custom spoke address.
src/svm/eventsClient.ts
Outdated
options: GetSignaturesForAddressConfig = { limit: 1000 }, | ||
finality: Commitment = "confirmed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to pass separate commitment in the finality
parameter (used in getTransaction
calls) from options.commitment
that gets used in getSignaturesForAddress
calls?
src/svm/eventsClient.ts
Outdated
const events = await this.readEventsFromSignature(signatureTransaction.signature, program, anchorIdl, finality); | ||
return events.map((event) => ({ | ||
...event, | ||
confirmationStatus: signatureTransaction.confirmationStatus || "Unknown", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe pass the Commitment | null
type as is?
src/svm/eventsClient.ts
Outdated
return events.map((event) => ({ | ||
...event, | ||
confirmationStatus: signatureTransaction.confirmationStatus || "Unknown", | ||
blockTime: signatureTransaction.blockTime || unixTimestamp(BigInt(0)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe pass UnixTimestamp | null
type as is?
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
|
||
while (hasMoreSignatures) { | ||
const signatures: GetSignaturesForAddressApiResponse = await this.rpc | ||
.getSignaturesForAddress(this.svmSpokeAddress, currentOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a signature
any transaction sent to the svmSpokeAddress
?
options: GetSignaturesForAddressConfig = { limit: 1000, commitment: "confirmed" } | ||
): Promise<EventWithData<T>[]> { | ||
const events = await this.queryAllEvents(fromSlot, toSlot, options); | ||
return events.filter((event) => event.name === eventName) as EventWithData<T>[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's no way to query the rpc
for just the eventName
? You always need to query all events and then filter from there?
Seems expensive but if we're smart about setting from/toSlot we can cache the results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just left some questions for my own knowledge
Changes proposed in this PR:
How to use it: