-
Notifications
You must be signed in to change notification settings - Fork 92
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
Changed tag format #1238
Changed tag format #1238
Conversation
0b185dc
to
6eb9068
Compare
6eb9068
to
9dfcd13
Compare
Codecov Report
@@ Coverage Diff @@
## conduit #1238 +/- ##
===========================================
- Coverage 62.09% 60.16% -1.94%
===========================================
Files 70 73 +3
Lines 9387 9857 +470
===========================================
+ Hits 5829 5930 +101
- Misses 3059 3419 +360
- Partials 499 508 +9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
"sort" | ||
"strings" | ||
|
||
"github.com/algorand/go-algorand/data/transactions" |
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.
Does this work if you use the transaction object from the go SDK?
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.
Perhaps. Is it easy to test by importing the sdk?
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.
Indexer uses the SDK, so I think you could swap in the other type: https://github.com/algorand/go-algorand-sdk/blob/develop/types/transaction.go#L25
It looks like SignedTxnWithAD
isn't available, but I'm not sure that one is especially useful so maybe this is OK?
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.
AD contains information regarding the application ID for ASA's or apps (at least according to the comments) as well rewards applied to the sender/receiver. Is that used by customers?
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.
Good point about the IDs, that might be the only place it is available when things are being created.
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. Suggested minor code simplifications.
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
Sorry, which issue is this actually resolving? |
var exp Expression | ||
|
||
switch targetKind { | ||
case reflect.TypeOf(basics.MicroAlgos{}).Kind(): |
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.
You could put this check in the generator. If the leaf type is MicroAlgos add the extra Raw reference:
case "txn.fee":
- return &input.SignedTxnWithAD.SignedTxn.Txn.Header.Fee, nil
+ return &input.SignedTxnWithAD.SignedTxn.Txn.Header.Fee.Raw, nil
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
Resolves #1238
Changes tag format and adds numerical filtering