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

Improving the official APIs? #23

Closed
ivnsch opened this issue May 24, 2021 · 2 comments
Closed

Improving the official APIs? #23

ivnsch opened this issue May 24, 2021 · 2 comments

Comments

@ivnsch
Copy link
Contributor

ivnsch commented May 24, 2021

While implementing a feature, I've stumbled upon some awkward parts in the official SDKs (Java and Go at least). This includes public APIs.

I wonder how closely we want to follow the public APIs and implementations? Following strictly at least the public APIs has a couple of benefits, but it's a bit limiting if we want to improve them or make them more idiomatic for Rust.

This was referenced May 24, 2021
@ivnsch
Copy link
Contributor Author

ivnsch commented May 25, 2021

A partly "Rust specific" example:

LogicSignature is modeled after the algod REST API:

pub struct LogicSignature {
    #[serde(rename = "l")]
    pub logic: Vec<u8>,

    pub sig: Option<Signature>,
    pub msig: Option<MultisigSignature>,

    #[serde(rename = "arg")]
    pub args: Vec<Vec<u8>>,
}

this struct is used in the domain too, which isn't ideal. As far I understand having both sig and msig set is illegal (it at least fails the verification). A sum type would represent this better:

enum Sig {
    Sig(Signature),
    Msig(MultisigSignature),
}

New domain type:

pub struct LogicSignature {
    pub logic: Vec<u8>,
    pub sig: Option<Sig>,
    pub args: Vec<Vec<u8>>,
}

Now we can't have the illegal state. It could be expanded further, to not need an optional:

enum Sig {
    ContractAccount
    Sig(Signature),
    Msig(MultisigSignature),
}

pub struct LogicSignature {
    pub logic: Vec<u8>,
    pub sig: Sig,
    pub args: Vec<Vec<u8>>,
}

And now we can remove already almost half of the Java SDK's verification method and related Unit Tests

The problem with these type of changes is that e.g. Java doesn't have sum types, so they can't be proposed there. @manuelmauro thoughts?

@ivnsch
Copy link
Contributor Author

ivnsch commented May 28, 2021

Cleared internally:

  • The SDK will prioritize idiomatic Rust.
  • For refactorings not related with above, we will submit issues to the Go SDK, when it's possible to keep the APIs similar.

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

No branches or pull requests

1 participant