-
Notifications
You must be signed in to change notification settings - Fork 37
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
Contract wrapper #9
Conversation
if (type instanceof U8Type) { | ||
return new U8Value(number); | ||
} | ||
if (type instanceof I8Type) { | ||
return new I8Value(number); | ||
} | ||
if (type instanceof U16Type) { | ||
return new U16Value(number); | ||
} | ||
if (type instanceof I16Type) { | ||
return new I16Value(number); | ||
} | ||
if (type instanceof U32Type) { | ||
return new U32Value(number); | ||
} | ||
if (type instanceof I32Type) { | ||
return new I32Value(number); | ||
} | ||
if (type instanceof U64Type) { | ||
return new U64Value(number); | ||
} | ||
if (type instanceof I64Type) { | ||
return new I64Value(number); | ||
} | ||
if (type instanceof BigUIntType) { | ||
return new BigUIntValue(number); | ||
} | ||
if (type instanceof BigIntType) { | ||
return new BigIntValue(number); | ||
} |
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.
how about a switch? https://log.havrlant.cz/typescript-switch-instanceof/
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.
Done
src/esdtToken.ts
Outdated
FungibleESDT, | ||
SemiFungibleESDT, | ||
NonFungibleESDT |
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.
Consider renaming these to Fungible
, Semifungible
, Nonfunginble
, i.e. without the ESDT
suffix and without capitalizing the word "fungible" when prefixed.
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.
Done
src/esdtToken.ts
Outdated
FungibleESDT, | ||
SemiFungibleESDT, | ||
NonFungibleESDT | ||
} | ||
|
||
export class ESDTToken { |
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.
Optional, but consider renaming ESDTToken
to TokenDefinition
, or simply Token
. It's true that this class is used for ESDT, but also EGLD, which is not an ESDT. Moreover, ESDT means "Elrond Standard Digital Token", so the "T" in ESDT already means "Token". ESDTToken
is redundant.
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.
True 😄
Renamed to Token
src/esdtToken.ts
Outdated
let [tokenName, tokenType, owner, totalMinted, totalBurned, ...propertiesBuffers] = results; | ||
let properties = parseTokenProperties(propertiesBuffers); |
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.
Consider extracting these two lines into a separate function which takes results
and returns properties
.
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 end up unpacking and then re-packing the first arguments when I extract those lines into a separate function:
function parseTokenPropertiesFromResults(results: any[]): { tokenName: any, tokenType: any, owner: any, totalMinted: any, totalBurned: any, properties: Record<string, any> } {
let [tokenName, tokenType, owner, totalMinted, totalBurned, ...propertiesBuffers] = results;
let properties = parseTokenProperties(propertiesBuffers);
return { tokenName, tokenType, owner, totalMinted, totalBurned, properties };
}
I think it's fine as it is right now.
src/esdtToken.ts
Outdated
canChangeOwner: response.canChangeOwner, | ||
canPause: response.canPause, | ||
canFreeze: response.canFreeze, | ||
canWipe: response.canWipe, |
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.
Should the properties canAddSpecialRoles
, canTransferNftCreateRole
, nftCreateStopped
and wiped
be also set here? Are they available in response
?
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.
At the moment they aren't: https://testnet-api.elrond.com/tokens
src/esdtToken.ts
Outdated
} | ||
|
||
function parseValue(value: string): any { | ||
switch (value) { |
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.
Before this switch
, consider adding an if
that checks if the value
is strictly an alphanumeric string (without the decimal point), and if it is, create a BigNumber
. Otherwise, go to the switch
, but on the default
branch, throw an exception (something like "unknown property").
This way, a malformed string will be prevented from reaching the BigNumber()
constructor.
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 function is used to check the results of the query to getTokenProperties
, which, for its properties has values of the form: NumDecimals-2
, IsPaused-false
.
Because they come from the ESDT system smart contract, I don't expect them to be malformatted.
|
||
function constructorFromJSON(json: any): EndpointDefinition | null { | ||
// if the JSON "constructor" field is missing | ||
if (json.constructor == Object.constructor) { |
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.
It's safer to test for json.constructor.inputs
and json.constructor.outputs
. If either of them is missing, then there's no definition in the ABI for the contract constructor.
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.
Done.
* Returns whether two objects have the same value. | ||
*/ | ||
equals(other: StringValue): boolean { | ||
return this.value == other.value; |
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.
For extra safety, use ===
here.
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.
Done. There's a linter check for this, but it's explicitly disabled at the moment. It should be re-enabled at some point.
let symbolsRegex = /(:|\{|\}|,|\s)/; | ||
let tokens = jsoned.split(symbolsRegex).filter((token) => token); | ||
jsoned = tokens.map((token) => (symbolsRegex.test(token) ? token : `"${token}"`)).join(""); | ||
let symbolsRegex = /(:|\{|\}|,|(?<!utf\-8)\s)/; |
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 mean "a symbol is one of :{},
and space (\s
), but up to and excluding utf-8
"?
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 utf-8 string
is a single type but because this code was splitting by space, I had to add an exception.
The (?<!utf\-8)
is a negative look-ahead.
I added a comment for this in the code.
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 would recommend moving these lines to a method called splitBySpaceExceptUTF8()
, then adding a few small unit tests just for 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.
I can't neatly split this into a separate function (since the symbolsRegex
is used in 2 places, and bringing both lines into the function would bring the rest of the code in as well, because of the re-used tokens
and jsoned
).
However, this function is well tested through the parse
method. I added a test-case for the utf-8 string
to the already existing tests in typeExpressionParser.spec.ts
.
export type Method<ReturnType> = (...args: any[]) => ReturnType; | ||
export type Methods<ReturnType> = Record<string, Method<ReturnType>>; | ||
|
||
export function generateMethods<ThisType, ReturnType>(this_: ThisType, abi: SmartContractAbi, endpointHandler: EndpointHandler<ThisType, ReturnType>): Methods<ReturnType> { |
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.
Please format as one parameter per line, for readability.
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.
Done
// A function may have one of the following formats: | ||
// f(arg1, arg2, optional<arg3>, optional<arg4>) returns { min: 2, max: 4, variadic: false } | ||
// f(arg1, variadic<bytes>) returns { min: 1, max: Infinity, variadic: true } | ||
// f(arg1, arg2, optional<arg3>, arg4, optional<arg5>, variadic<bytes>) returns { min: 4, max: Infinity, variadic: true } |
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.
Shouldn't this return { min: 2, max: Infinity, variadic: true }
? Only the first 2 arguments are required, the rest are optional.
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.
True. I corrected the comment.
@@ -94,7 +96,7 @@ export class WalletProvider implements IDappProvider { | |||
/** | |||
* Fetches the login hook url and redirects the client to the wallet login. | |||
*/ | |||
async login(options?: { callbackUrl?: string }): Promise<string> { | |||
async login(options?: { callbackUrl?: string; token?: string }): Promise<string> { |
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.
Line 141 will overwrite any value provided to this argument here. Is this intentional?
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 change was displayed due to more commits being made on the development branch and it's not something I wrote.
Basically, I had to do a merge from development first.
let symbolsRegex = /(:|\{|\}|,|\s)/; | ||
let tokens = jsoned.split(symbolsRegex).filter((token) => token); | ||
jsoned = tokens.map((token) => (symbolsRegex.test(token) ? token : `"${token}"`)).join(""); | ||
let symbolsRegex = /(:|\{|\}|,|(?<!utf\-8)\s)/; |
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 would recommend moving these lines to a method called splitBySpaceExceptUTF8()
, then adding a few small unit tests just for it.
src/dapp/walletProvider.ts
Outdated
if (!this.isValidWalletSource(ev.origin)) { | ||
return; | ||
} | ||
|
||
const { data } = ev; | ||
if (data.type !== DAPP_MESSAGE_SIGN_TRANSACTION_URL) { | ||
return; | ||
} |
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.
Should these be return reject(...);
instead?
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.
Same as above - these were changes from the development branch.
src/dapp/walletProvider.ts
Outdated
plainTransaction["value"] = transaction.getValue().toString(); | ||
plainTransaction["gasPrice"] = transaction.getGasPrice().valueOf(); | ||
plainTransaction["gasLimit"] = transaction.getGasLimit().valueOf(); | ||
|
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.
Chain ID and transaction spec version?
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.
Same as above - these were changes from the development branch.
Better handle transaction completion: proxy vs. API
fixed changelog, export encryptor/decryptor and prepared release
fixed changelog, export encryptor/decryptor and prepared release
Implements a contract wrapper which allows an user to interact with smart contracts, with the main focus being on simplicity.
The examples which this pull request enables can be found in: multiversx/mx-sdk-rs#268
ContractWrapper
- a wrapper over a smart contractawait adder.gas(3_000_000).call.add(30);
let sum = await adder.query.getSum();
FormattedCall
objectmultisig
smart contract, where actions are "proposed"let adder = await erdSys.loadWrapper("contracts/examples/adder");
let answer = await erdSys.loadWrapper("src/testdata", "answer");
.value(MyToken(amount))
ESDTTransfer
orESDTNFTTransfer
)SendContext
andChainSendContext
which help in keeping track of transaction propertiesprovider
,logger
,sender
,address
,gas
value
SystemWrapper
class which encapsulates the builtin functions and ESDT system smart contract and provides a more intuitive APIBalance
class - generalized it to accept other currencies as wellBalanceBuilder
which can be used to createBalance
instances for any EGLD or ESDTMyToken(10.0)
MyToken.raw(amount)
IProvider
(ProxyProvider
) endpointsaddress/:address/esdt
-getAddressEsdtList
address/:address/esdt/:tokenIdentifier
-getAddressEsdt
address/:address/nft/:tokenIdentifier/nonce/:nonce
-getAddressEsdt
NativeSerializer
namespace which allow easier conversion from javascript native types to the typed value system of erdjsmyMethod(1_000_000)
instead ofmyMethod(new U32Value(1_000_000))
ArgumentErrorContext
print
,printList
,minutesToNonce
,now
,hours
,minutes
utf8 string
parsingnpm run lint
commandchooseProvider
function which accepts one oflocal-testnet
,elrond-testnet
,elrond-devnet
,elrond-mainnet
getDevnetProvider
,getTestnetProvider
,getMainnetProvider
because of confusing naming (getDevnetProvider
was returning the proxy forhttp://localhost:7950
)Result
ImmediateResult
toTypedResult
and re-use it for both immediate results and resulting calls