-
Notifications
You must be signed in to change notification settings - Fork 38
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
refactor: strategies #328
refactor: strategies #328
Conversation
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
My comments are mostly about the DX and UX for Bitcoin address validation.
If we don't have any opinions about what Bitcoin addresses are valid (e.g. p2tr), then I suppose we can ignore most of what I said and leave it up to the UI's implementation. For example, here's where our UI uses sats-wagmi to give the user a descriptive error for a taproot address.
Here's another example of stakingrewards.com just throwing up a box to make the user verify they don't hold ordinals in the address they use.
// NOTE: we also support Bitcoin testnet | ||
BITCOIN = "bitcoin", |
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.
If a dev is going through the testnet workflow, they're meant to put fromChain: "Bitcoin",
in their quoteParams
? I found it clunky not to differentiate between Bitcoin mainnet and testnet, even though it seems like we're halfway to cleverly abstracting the need to be explicit about that away from the dev
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.
Do you have a suggestion for a well-known chain slug? Happy to change it but couldn't think of anything
gasRefill?: number, | ||
gasRefill?: number; | ||
/** @description Wallet public key on source chain */ | ||
fromUserPublicKey?: 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.
Under what conditions would the caller need to supply fromUserPublicKey
? Are there already descriptive error messages for these edge cases?
Does it have to do with different Bitcoin address types? e.g. is fromUserAddress: "bc1qafk4yhqvj4wep57m62dgrmutldusqde8adh20d",
in quoteParams
sufficient?
TIA for the response; I suspect it would be useful in the docs.
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.
Only required if the user is using P2SH-P2WPKH, we have some descriptive errors but agree we should note in the docs
name: string; | ||
/** Format: uri */ | ||
logo: string; | ||
monetization: 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.
Nice. I like introducing the monetization
key now to align with Swing and set ourselves up for turning this on later.
sdk/test/gateway.test.ts
Outdated
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.
Related to my Bitcoin address question above, I wonder if the DX (i.e. descriptive errors) around multiple Bitcoin address types could be useful here.
Most (all?) of the server-side testing uses the same mock Bitcoin address.
fn mock_bitcoin_address() -> bitcoin::Address {
bitcoin::Address::from_str("bcrt1qj4ymqevserwz045556l6qjycxnumcwwk09t3af")
.unwrap()
.assume_checked()
}
I'm happy to help with test coverage (and perhaps the errors, if needed) if you can help me catch up to how we currently handle Bitcoin address validation.
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.
Also approving so you can merge
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
67ad99f
to
0123116
Compare
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
GatewayQuoteParams
to be more likeTransferParams
Token
type from superchain repogetStrategies
with Swing typesProcess to integrate e.g. Pell Network without an output token: