-
Notifications
You must be signed in to change notification settings - Fork 5
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(sdk): Add support for Ordexer #107
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Co-authored-by: Benedict Pak <[email protected]>
export type DataSourceType = JsonRpcDatasource | OrdexerDatasource; | ||
|
||
export const DataSource = { | ||
Jsonrpc: JsonRpcDatasource, |
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 think it would be better to change it to ordit
instead of Jsonrpc
.
}, | ||
}, | ||
}; | ||
|
||
export const ORDEXER_AUTH_TOKEN = ""; |
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.
can you make this configuration customizable to the client?, maybe put the config/opts when user defining the datasource
const response: GetInscriptionResponse = await ordexer[this.network].get( | ||
`/ordinals/inscriptions/${id}`, | ||
); | ||
|
||
const mediaContent: string = await this.getInscriptionContent({ id }); |
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.
const response: GetInscriptionResponse = await ordexer[this.network].get( | |
`/ordinals/inscriptions/${id}`, | |
); | |
const mediaContent: string = await this.getInscriptionContent({ id }); | |
const [response, mediaContent] = await Promise.all([ | |
ordexer[this.network].get( | |
`/ordinals/inscriptions/${id}`, | |
) as Promise<GetInscriptionResponse>, | |
this.getInscriptionContent({ id }), | |
]); | |
mediaType: inscription.mediaType ?? "", | ||
mediaSize: inscription.mediaSize, | ||
mediaContent: await this.getInscriptionContent({ | ||
id: inscription.inscriptionId, |
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.
the throttle limit would be exceeded if there are too many requests, and also network requests are doubling. i think it's better to include mediaContentBase64 in the ordexer API
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'm thinking if we actually need this here, since this is GetTransaction
. I'm not sure if mediaContent
would be needed in this case.
@@ -95,7 +95,7 @@ export type Vin = { | |||
/** | |||
* Hex-encoded witness data | |||
*/ | |||
txinwitness: string[]; | |||
txinwitness?: string[]; |
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 think it's better to make empty array on ordexer side instead of changing the type definition
@@ -30,7 +30,7 @@ export type Vout = { | |||
/** | |||
* Is spent (Ordit RPC), can be Outpoint `{txid}:{vout}`, or `false` if not spent | |||
*/ | |||
spent: string | false; | |||
spent: boolean; |
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.
hmm, i think ordexer also should return {txid}:{vout} | false
, is the ordit/client(ordzaar) aware about this changes?
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.
On Ordit, it's string | false
but it's always a boolean, that is set to true
when the output is spent.
Setting to true: https://github.com/sadoprotocol/ordit/blob/54be99d660e9995d14c14f60e61295ee1a3f16b9/src/Database/Output/Methods.ts#L232
DB Query:
https://github.com/sadoprotocol/ordit/blob/54be99d660e9995d14c14f60e61295ee1a3f16b9/src/Database/Output/Utilities.ts#L6
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.
On Ordexer, the schema stores it as a boolean: https://github.com/ordzaar/ordexer/blob/29a2424d50e6da02c6d40ef9f2da297a139c1104/packages/db/prisma/schema.prisma#L36
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 changing it here just makes it consistent across Ordit, Ordexer, and Ordit SDK.
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.
need to check whether the client has used the spent string or not, this transaction API will return string | boolean
https://github.com/sadoprotocol/ordit/blob/main/src/Utilities/Transaction.ts#L69C45-L69C59
maybe add spentVin
for in the ordexer response if that necessary, and parse the response in sdk client without changing the types
import { Params, removeTrailingSlash } from "../utils"; | ||
|
||
class OrdexerApi { | ||
constructor(readonly url: string) {} |
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.
how about accepting apiKey as a param here?
(readonly url: string, params: { apiKey: string })
baseURL: this.url, | ||
url: endpoint, | ||
headers: { | ||
Authorization: `Bearer ${ORDEXER_AUTH_TOKEN}`, |
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 accept auth token from constructor
What this PR does / why we need it:
Which issue(s) does this PR fixes?:
Fixes #
Additional comments?: