Skip to content
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

Implement 'publishVersionIntent' #35

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@izqui
Copy link
Member

izqui commented Apr 3, 2019

Implements a new publishVersionIntent function that returns an aragon.js intent object that can be used to publish a new version using transaction pathing.

publishIntent now uses publishVersionIntent and still generates the direct transaction to publish a version, making the change fully backwards compatible.

Closes #13

Show resolved Hide resolved src/index.js Outdated
Update src/index.js
Co-Authored-By: sohkai <qisheng.brett.sun@gmail.com>
@@ -226,15 +226,18 @@ module.exports = (web3, options = {}) => {
repo.methods.isValidBump(formatVersion(fromVersion), formatVersion(toVersion)).call()
))
},
getKernel (app) {

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Seems unnecessary to introduce this into the public API. Could we move it up above the return so it's an internal method?

@@ -243,6 +246,45 @@ module.exports = (web3, options = {}) => {
* @return {Promise} A promise that resolves to a raw transaction

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Can we add the from param to the JSDoc?

data: call.encodeABI(),
gas: Math.round(await call.estimateGas({ from }) * GAS_FUZZ_FACTOR),
gasPrice: web3.utils.toWei('10', 'gwei'),
nonce: await web3.eth.getTransactionCount(manager)

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Is the nonce necessary here? If we let the signer handle this, would it allow us to publish multiple packages with the same key "in parallel" (e.g. without having to wait for the previous publish tx to be mined)?

},

/**
* Publishes a new version (`version`) of `appId` using storage provider `provider`.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member
Suggested change
* Publishes a new version (`version`) of `appId` using storage provider `provider`.
* Create an intent to publish a new version (`version`) of `appId` using storage provider `provider`.
return {
dao: await this.getKernel(repo),
proxyAddress: repo.options.address,
name: 'newVersion',

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Perhaps we could use methodName instead?

// If the repo does not exist yet, the intent will be for creating a repo with the first version
const repoRegistry = await this.getRepoRegistry(appId)
.catch(() => {
throw new Error(`Repository ${appId} does not exist and it's registry does not exist`)

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member
Suggested change
throw new Error(`Repository ${appId} does not exist and it's registry does not exist`)
throw new Error(`Repository ${appId} does not exist and its registry does not exist`)
* Publishes a new version (`version`) of `appId` using storage provider `provider`.
*
* If the destination repository does not exist, the intent will be for creating a new
* repository with an initial version.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Is it a lot easier for the CLI to use a single function to do both version publishes as well as repo creations?

Really seems like there's two concerns mixed here (you can immedately tell by the manager being the first argument, but it not being used at all on the "normal" path).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.