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

Format binary data as hex in sql/satn #521

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Format binary data as hex in sql/satn #521

merged 5 commits into from
Nov 29, 2023

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Nov 3, 2023

Description of Changes

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Expected complexity level and risk

1

let dbs = cdb.get_databases()?;

assert_eq!(dbs.len(), 1);
assert_eq!(dbs[0].id, id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are you testing here? Shouldn't you be checking identity rather than id?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Josh, not sure what is going on here.

]);
let data = r#"
{
"foo": 42,
"bar": "404040FFFF0A48656C6C6F",
"baz": ["heyyyyyy", "hooo"],
"quux": { "Hash": "54a3e6d2b0959deaacf102292b1cbd6fcbb8cf237f73306e27ed82c3153878aa" },
"and_peggy": { "some": 3.141592653589793238426 }
"and_peggy": { "some": 3.141592653589793238426 },
"identity": ["0000000000000000000000000000000000000000000000000000000000000000"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this an array of strings and not just a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the internal representation of identity is AlgebraicType::product([("__identity_bytes", AlgebraicType::bytes())])

Keeping the same structure here means avoids having a discrepancy between bsatn, json, satn.

crates/lib/tests/snapshots/serde__json_mappings.snap Outdated Show resolved Hide resolved
crates/sats/src/satn.rs Outdated Show resolved Hide resolved
let dbs = cdb.get_databases()?;

assert_eq!(dbs.len(), 1);
assert_eq!(dbs[0].id, id);
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Josh, not sure what is going on here.

@mamcx mamcx force-pushed the mamcx/identity-hex branch from d7d2507 to 36dfbdf Compare November 6, 2023 15:42
@kulakowski
Copy link
Contributor

@cloutiertyler I believe your requested change is in, if you don't mind poking it along

@mamcx mamcx self-assigned this Nov 29, 2023
@mamcx
Copy link
Contributor Author

mamcx commented Nov 29, 2023

This should close #506

Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
@mamcx mamcx merged commit 4d49b14 into master Nov 29, 2023
5 checks passed
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.

5 participants