-
Notifications
You must be signed in to change notification settings - Fork 486
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
BoxesResponse: Fold names into top-level object #4249
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/avm-box #4249 +/- ##
===================================================
- Coverage 54.58% 54.57% -0.01%
===================================================
Files 393 393
Lines 49283 49284 +1
===================================================
- Hits 26899 26898 -1
- Misses 20115 20117 +2
Partials 2269 2269
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems a reasonable change, I am not tagged to review so I just comment here.
It just surprise me that the generated model cannot handle response type of array of strings, and I wrote most of the code here, so I kinda feel guilty about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, as discussed off line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
free approval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Due to algorand/generator#36, it's not possible to generate working Java and Go SDK changes prior to the PR (see algorand/go-algorand-sdk#340 as an example).
Consequently, the PR proposes re-defining the response for
/v2/applications/{application-id}/boxes
to be compatible with as-is generator capabilities.Notes:
Box
definition exists (returning name + value), the PR proposes a new definition:BoxDescriptor
. I'm open to different names. The intent is to use a name that may be extended with new fields in the future.BoxDescriptor
. The approach is a better fit for today's model. However, if we later modify the response type, it'll introduce a backwards incompatible change.