-
Notifications
You must be signed in to change notification settings - Fork 141
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
New Feature: Adding methods to use the simulate endpoint #420
Conversation
@@ -1,6 +1,6 @@ | |||
# Configs for testing repo download: | |||
SDK_TESTING_URL="https://github.com/algorand/algorand-sdk-testing" | |||
SDK_TESTING_BRANCH="master" | |||
SDK_TESTING_BRANCH="simulate-endpoint-tests" |
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.
remember to change this back
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.
i looked at the trap ray
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.
Sorry for some of the sudden upstream SDK step-related changes - I commented the places where you might need to update 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 👍 minor comments and ready to approve when SDK testing PR is merged and the branch is reverted back to master.
def simulate_atc_failure(context, group, path, message): | ||
resp: SimulateAtomicTransactionResponse = context.simulate_atc_response | ||
group_idx: int = int(group) | ||
fail_path = ",".join( |
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.
nit:
fail_path = ",".join( | |
fail_path: str = ",".join( |
|
||
for i, tx_id in enumerate(self.tx_ids): | ||
tx_results = [ | ||
client.pending_transaction_info(tx_id) for tx_id in self.tx_ids |
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.
caveat/nit: I don't think we need to get the txn info for every transaction, just the ones with method calls (self.method_dict
). If the call overhead isn't too bad, we could keep as written in the current PR.
adding tests for empty signer simulation
def sign_transactions( | ||
self, txn_group: List[transaction.Transaction], indexes: List[int] | ||
) -> List[GenericSignedTransaction]: | ||
stxns = [] |
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.
- stxns = []
+ stxns: List[GenericSignedTransaction] = []
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.
the linter is complaining error: Incompatible return value type
Adding Simulate endpoint support to algod client
Also added simulate as a method on the AtomicTransactionComposer to allow an api like: