-
Notifications
You must be signed in to change notification settings - Fork 209
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
3.0.0: Refactor Transaction
class
#854
Changes from 15 commits
476902c
ced00e0
be5e469
63990dc
790e95f
e630d7b
034a7c4
11331aa
254a9a5
6d9b5ae
804ccf2
2e51a5d
93a7ea9
ba432ca
9d205bd
ffc585b
1e3371a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ version: 2.1 | |
orbs: | ||
node: circleci/[email protected] | ||
slack: circleci/[email protected] | ||
browser-tools: circleci/[email protected].3 | ||
browser-tools: circleci/[email protected].6 | ||
gh-pages: sugarshin/[email protected] | ||
|
||
parameters: | ||
|
@@ -89,7 +89,8 @@ jobs: | |
$(lsb_release -cs) stable" | $SUDO tee /etc/apt/sources.list.d/docker.list > /dev/null | ||
$SUDO apt update | ||
$SUDO apt -y install docker-ce docker-ce-cli containerd.io | ||
- browser-tools/install-browser-tools | ||
- browser-tools/install-browser-tools: | ||
replace-existing-chrome: true | ||
- run: | ||
name: << parameters.browser >> test | ||
command: | | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,13 +51,12 @@ async function main() { | |
// example: ATOMIC_GROUP_SEND | ||
|
||
// example: CONST_MIN_FEE | ||
const minFee = algosdk.ALGORAND_MIN_TX_FEE; | ||
console.log(minFee); | ||
// This SDK does not expose a constant for the minimum fee | ||
// example: CONST_MIN_FEE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we remove the comment references to CONST_MIN_FEE entirely? Also, there seems to be two identical lines for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These comments are used to populate snippets of code in the dev site. This specific one is used here: https://developer.algorand.org/docs/get-details/dapps/smart-contracts/guidelines/#get-minimum-fee-off-chain-with-sdk I think we want to preserve JS in that tab, but just have a comment saying a hard coded min fee is not available. That's currently what happens for the "using an algod API" JS snippet on that page |
||
|
||
// example: TRANSACTION_FEE_OVERRIDE | ||
const sp = await client.getTransactionParams().do(); | ||
sp.fee = 2 * minFee; | ||
sp.fee = BigInt(2) * sp.minFee; | ||
jannotti marked this conversation as resolved.
Show resolved
Hide resolved
|
||
sp.flatFee = true; | ||
// example: TRANSACTION_FEE_OVERRIDE | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
import * as nacl from './nacl/naclWrappers.js'; | ||
import * as address from './encoding/address.js'; | ||
import { Address } from './encoding/address.js'; | ||
import Account from './types/account.js'; | ||
|
||
/** | ||
* generateAccount returns a new Algorand address and its corresponding secret key | ||
*/ | ||
export default function generateAccount(): Account { | ||
const keys = nacl.keyPair(); | ||
const encodedPk = address.encodeAddress(keys.publicKey); | ||
return { addr: encodedPk, sk: keys.secretKey }; | ||
const addr = new Address(keys.publicKey); | ||
return { addr, sk: keys.secretKey }; | ||
} |
This file was deleted.
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.
This ensures we always get the latest stable chrome version, which our browser testing relies on
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.
Is there a reason to intentionally test on a cross-range of versions? Wondering if instead of doing latest, we should do latest, plus X releases 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.
There may be some value in selecting an older chrome version to test against, so that we can be sure the code isn't relying on APIs/behaviors that we think are too new.
But in reality it's easiest to test against latest and we still benefit a lot from having this.
I only included this change because for some reason circle stopped installing the latest chrome by default, which broke our test