Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Feature/0.3 #102

Merged
merged 7 commits into from
Dec 20, 2018
Merged

Feature/0.3 #102

merged 7 commits into from
Dec 20, 2018

Conversation

iantanwx
Copy link
Contributor

@iantanwx iantanwx commented Dec 17, 2018

Description

This PR adds a number of requested features. Unfortunately, it also entails breaking changes.

  • add more flexibility to Contract by allowing manual specification of pubKey (sender) and nonce
  • add maxAttempts and interval to Contract methods
  • return the underlying Transaction from Contract.prototype.deploy
  • adds toQa and fromQa utility functions to @zilliqa-js/util, as all units are now qa
  • make RPCResponse types comply with the changes to libServer on the protocol side
  • fix an issue where const enum declarations were being nuked in the compiled bundle due to failure to pass in preserveConstEnums as a compiler option

Contract.prototype.deploy more flexible

This addresses a long-standing issue where developers were not able to
specify any address other than the default one when deploy/calling smart
contracts. Tests have also been updated to provide `_scilla_version`,
which is now a required parameter.
@codecov-io
Copy link

codecov-io commented Dec 17, 2018

Codecov Report

Merging #102 into master will decrease coverage by 0.65%.
The diff coverage is 79.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
- Coverage   78.71%   78.06%   -0.66%     
==========================================
  Files          33       36       +3     
  Lines         794      939     +145     
  Branches      106      127      +21     
==========================================
+ Hits          625      733     +108     
- Misses        168      204      +36     
- Partials        1        2       +1
Impacted Files Coverage Δ
packages/zilliqa-js-contract/src/types.ts 100% <ø> (ø)
packages/zilliqa-js-blockchain/src/util.ts 16.66% <ø> (-11.91%) ⬇️
packages/zilliqa-js-contract/src/contract.ts 79.31% <100%> (+1.12%) ⬆️
packages/zilliqa-js-core/src/net.ts 98.21% <100%> (+9.32%) ⬆️
packages/zilliqa-js-util/src/index.ts 100% <100%> (ø) ⬆️
packages/zilliqa-js-account/src/transaction.ts 87.05% <100%> (-0.16%) ⬇️
packages/zilliqa-js-core/src/util.ts 63.63% <20%> (-36.37%) ⬇️
packages/zilliqa-js-blockchain/src/chain.ts 54.54% <36.36%> (ø) ⬆️
packages/zilliqa-js-util/src/unit.ts 80.95% <80.95%> (ø)
packages/zilliqa-js-core/src/errors.ts 0% <0%> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23966ec...4cf449e. Read the comment docs.

_tag: transition,
// TODO: this should be string, but is not yet supported by lookup.
params,
params: args,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the string supported by the current lookup now?

Copy link
Contributor

@edison0xyz edison0xyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I have some general questions

msg = {_tag : "Main"; _recipient : _sender; _amount : Uint128 0; code : set_hello_code};
msgs = one_msg msg;
send msgs
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to standardize all helloWorld contracts with events instead of send msg?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this test contract is kept in line with whatever is on master of https://github.com/Zilliqa/scilla.

packages/zilliqa-js-util/test/unit.spec.ts Show resolved Hide resolved
Copy link
Contributor

@mickys mickys left a 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.

Personally i would use some constants for the gasPrice / gasLimit in tests instead of relying on " new BN(1000) / Long.fromNumber(1000) " but that's more of a preference

@iantanwx iantanwx merged commit a197f6f into master Dec 20, 2018
@edison0xyz edison0xyz deleted the feature/0.3 branch October 9, 2019 14:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants