Skip to content

Create a raw DeepBook client#122

Open
0xaslan wants to merge 3 commits into
mainfrom
at/db-sdk-generalize
Open

Create a raw DeepBook client#122
0xaslan wants to merge 3 commits into
mainfrom
at/db-sdk-generalize

Conversation

@0xaslan

@0xaslan 0xaslan commented Mar 20, 2025

Copy link
Copy Markdown

Description

Changed all transaction builders to use raw values. For example, they take the poolAddress rather than the poolKey.
Updated the client to get addresses/types given keys.
Added a raw client that works with absolute values.

Test plan

Integration tests


@0xaslan 0xaslan requested a review from a team as a code owner March 20, 2025 14:15
@vercel

vercel Bot commented Mar 20, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-typescript-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 20, 2025 3:15pm

@0xaslan 0xaslan had a problem deploying to sui-typescript-aws-kms-test-env March 20, 2025 15:12 — with GitHub Actions Failure
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ 🦋 Changesets Warning: This PR has changes to public npm packages, but does not contain a changeset. You can create a changeset easily by running pnpm changeset in the root of the Sui repo, and following the prompts. If your change does not need a changeset (e.g. a documentation-only change), you can ignore this message. This warning will be removed when a changeset is added to this pull request.

Learn more about Changesets.

@tonylee08

Copy link
Copy Markdown
Contributor

@hayes-mysten We're restructuring the Deepbook SDK to better support permissionlessly created pools. Let us know if there are any suggestions you have regarding the design here

@hayes-mysten hayes-mysten left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a massive PR, so I haven't had a chance to review it in detail, but the patterns here are kinda confusing.

skimming though this here are the things that stand out:

  • This duplicates a lot of logic across clients, if we are going to have 2 separate clients for this, one implementation could at least use the other implementation internally while exposing a different public interface
  • The name of the raw client isn't very intuitive, and I think comes across as something that shouldn't be used
  • I am not clear on why this needs to be a separate client at all
  • We seem to have lost a lot of the patterns and conventions from the last time I reviewed this
    • Many methods have long lists of ordered arguments, which leads to bad DX
    • There is a big mix of casing conventions with lots of stuff being snake cased, which we don't usually use in TS
    • We are defining BCS types inline in methods, which seems like very weird pattern, and I was noticing that a lot of the types were being re-defined in multiple methods
    • Methods are being explicitly typed with return types like any or object when the correct types are easily accessible
    • Most return types are being defined inline, but I think for a lot of these methods, especially if we are implementing the same method in multiple classes, it would make more sense to define these types as named interfaces
    • We are converting to Number all over the place, which probably isn't safe
      • If we really want to convert to number (rather than something like bigint) and we know for sure we won't run into precision issues, we should use Number.parseInt

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.

3 participants