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: transparent bindings and bigints #919

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

Cifko
Copy link
Contributor

@Cifko Cifko commented Jan 29, 2024

Description

Handle the #[serde(transparent)] structs, and also fix the bigint. i64 and u64 were exported as bigint, instead of number. The number can hold anything in range(inclusive) from -(2^53-1)...(2^53-1). Anything above or below can be hold but it's not safe to be precise.

Motivation and Context

Some of the structures are #[serde(transparent)] this is not recognized by the ts-rs crate. If the structure is "real struct" (e.g. struct A{b:u64} and not a struct A(u64), then we can call #[ts(flatten)] but you have to call it on every instance. And it will rewrite the variable e.g.

struct A{
  #[ts(flatten)]
  b:B
}

struct B {
  c:u8
}

Will be exported as

export interface A {
  c : number; // instead of b : number
}

So I have decided instead of flatten define the types by hand

How Has This Been Tested?

What process can a PR reviewer use to test or verify this change?

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Other - Please specify

Copy link

github-actions bot commented Jan 29, 2024

Test Results (CI)

436 tests  +356   436 ✅ +357   1h 42m 17s ⏱️ + 46m 3s
 57 suites + 41     0 💤 ±  0 
  2 files   +  1     0 ❌  -   1 

Results for commit 535551e. ± Comparison against base commit c29d5b8.

This pull request removes 1 and adds 357 tests. Note that renamed tests count towards both.
tari_dan_common_types ‑ shard::export_bindings_shard
Scenario: Claim and transfer confidential assets via wallet daemon: tests/features/wallet_daemon.feature:89:5
Scenario: Claim base layer burn funds with wallet daemon: tests/features/claim_burn.feature:6:3
Scenario: Claim validator fees: tests/features/claim_fees.feature:6:3
Scenario: Confidential transfer to unexisting account: tests/features/transfer.feature:163:3
Scenario: Counter template registration and invocation: tests/features/counter.feature:7:3
Scenario: Create account and transfer faucets via wallet daemon: tests/features/wallet_daemon.feature:7:5
Scenario: Create and mint account NFT: tests/features/wallet_daemon.feature:133:5
Scenario: Create resource and mint in one transaction: tests/features/nft.feature:73:3
Scenario: Double Claim base layer burn funds with wallet daemon. should fail: tests/features/claim_burn.feature:44:3
Scenario: Indexer GraphQL requests events over network substate indexing: tests/features/indexer.feature:118:3
…

♻️ This comment has been updated with latest results.

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Loosely tested

Perhaps just change the paths to be correct inside dan_layer so that they go into bindings in the git root. It might be better to package JRPC types for the VN, indexer and wallet in separate NPM packages but we can clean that stuff up at a later point

sdbondi
sdbondi previously approved these changes Feb 5, 2024
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Approving because we can clean the paths up later, since build.sh moves them

btw probably want to add npx infront of shx mv... I get a command not found error in build.sh but if the paths are fixed we can remove the mv commands anyway

@Cifko
Copy link
Contributor Author

Cifko commented Feb 6, 2024

Approving because we can clean the paths up later, since build.sh moves them

btw probably want to add npx infront of shx mv... I get a command not found error in build.sh but if the paths are fixed we can remove the mv commands anyway

done

@sdbondi sdbondi added this pull request to the merge queue Feb 7, 2024
Merged via the queue into tari-project:development with commit 0c6f6e9 Feb 7, 2024
11 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Feb 12, 2024
Description
---
The #919, #920 need to go in first. Then I will rebase this.
This PR removes all the any that could be replaced by our auto-generated
interfaces/types.
Now the jsonrpc calls is expecting the proper request and gives proper
response. So when the structure changes we will know.

Motivation and Context
---

How Has This Been Tested?
---
Running dan-testing with a wallet with some accounts and transactions.

What process can a PR reviewer use to test or verify this change?
---
Same as above, maybe there is something I haven't tested (looked at).
But at the first glance it's working as expected.

Breaking Changes
---

- [x] None
- [ ] Requires data directory to be deleted
- [ ] Other - Please specify

---------

Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants