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

GetApplicationBoxByName API #586

Merged
merged 35 commits into from
Jul 7, 2022
Merged

GetApplicationBoxByName API #586

merged 35 commits into from
Jul 7, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Jun 13, 2022

This PR introduces:

  • GetApplicationBoxByName endpoint for boxes
  • Generated Box model from OA2 spec
  • Cucumber integration and algod path tests

@algochoi algochoi changed the base branch from develop to feature/box-storage June 13, 2022 19:35
@algochoi algochoi marked this pull request as ready for review July 6, 2022 14:52
@algochoi algochoi requested review from barnjamin and jdtzmn July 6, 2022 15:38
Copy link
Contributor

@jdtzmn jdtzmn left a comment

Choose a reason for hiding this comment

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

Mostly looks good! Left a few questions which could lead to changes.

}

// name sets the box name in base64 encoded format.
name(name: Uint8Array) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why limit the name to Uint8Array and then encoding that as a base64 app arg as opposed to supporting the app arg format directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - We discussed a bit about it offline last week and came to two thoughts:

  1. Since appArgs are in Uint8Array in this SDK, it makes sense for box name to mimic the JS SDK format (rather than mimic the goal format)
  2. It's out of scope for this PR to include these conversion tools for now. We may revisit this in the future and provide functions to convert ints or addrs later for box names.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Thanks for the explanation!

@@ -127,6 +127,54 @@ module.exports = function getSteps(options) {

const { algod_token: algodToken, kmd_token: kmdToken } = options;

// String parsing helper methods
function processAppArgs(subArg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If not supporting passing app args directly through the name() method, consider exporting this method to make it easier to encode app args.

Copy link
Contributor

@jdtzmn jdtzmn left a comment

Choose a reason for hiding this comment

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

Looks good! Left one optional comment.

import HTTPClient from '../../client';
import IntDecoding from '../../../types/intDecoding';

export default class GetApplicationBoxByName extends JSONRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider typing the response with JSONRequest<types.Box>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@algochoi algochoi merged commit 077c572 into feature/box-storage Jul 7, 2022
@algochoi algochoi deleted the box-api branch July 7, 2022 20:56
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.

4 participants