Skip to content

Public offer strict encoding#197

Merged
h4sh3d merged 6 commits intofarcaster-project:mainfrom
Lederstrumpf:public_offer_id_strict_encoding
Dec 6, 2021
Merged

Public offer strict encoding#197
h4sh3d merged 6 commits intofarcaster-project:mainfrom
Lederstrumpf:public_offer_id_strict_encoding

Conversation

@Lederstrumpf
Copy link
Member

No description provided.

@Lederstrumpf Lederstrumpf force-pushed the public_offer_id_strict_encoding branch from e045876 to 465282c Compare December 6, 2021 13:04
@Lederstrumpf Lederstrumpf changed the title Public offer id strict encoding Public offer strict encoding Dec 6, 2021
@h4sh3d
Copy link
Member

h4sh3d commented Dec 6, 2021

I'm not convinced the best way for public offer serde is with the string, if we want a string we use a String type and have it with offer.to_string() when we want to serialize the encoded offer. I'd prefer having proper serde impl for all the fields, this is something I've worked on but not finished.

@Lederstrumpf Lederstrumpf force-pushed the public_offer_id_strict_encoding branch from 465282c to cc56dfb Compare December 6, 2021 13:07
@Lederstrumpf
Copy link
Member Author

Fully agree it's not the best way. I suggest we use this and switch to your proper serde impl once that's done, unless you can finish that quickly?

@h4sh3d
Copy link
Member

h4sh3d commented Dec 6, 2021

Fair enough, let's add this and release 0.4.3, then add proper support in 0.5.

@Lederstrumpf Lederstrumpf marked this pull request as draft December 6, 2021 13:11
@Lederstrumpf Lederstrumpf force-pushed the public_offer_id_strict_encoding branch from aa3d3ff to cb5eae0 Compare December 6, 2021 14:13
let encoded = base58_monero::encode_check(consensus::serialize(self).as_ref())
.expect("Encoding in base58 check works");
s.push_str(&encoded);
serializer.serialize_str(s.as_ref())
Copy link
Member

Choose a reason for hiding this comment

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

This code will also run if we use to_string, we should use it to maintain only one interface IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed - updated

@Lederstrumpf Lederstrumpf force-pushed the public_offer_id_strict_encoding branch from d5d7d05 to f1072a4 Compare December 6, 2021 14:22
@Lederstrumpf Lederstrumpf force-pushed the public_offer_id_strict_encoding branch from f1072a4 to a380795 Compare December 6, 2021 14:25
@Lederstrumpf Lederstrumpf marked this pull request as ready for review December 6, 2021 14:26
@h4sh3d h4sh3d merged commit 37cfa77 into farcaster-project:main Dec 6, 2021
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.

2 participants