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

Merged
merged 4 commits into from Apr 26, 2019
Merged
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -226,15 +226,18 @@ module.exports = (web3, options = {}) => {
repo.methods.isValidBump(formatVersion(fromVersion), formatVersion(toVersion)).call()
))
},
getKernel (app) {
This conversation was marked as resolved by 0x6431346e

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?

return app.methods.kernel().call()
},
/**
* Publishes a new version (`version`) of `appId` using storage provider `provider`.
*
* If the destination repository does not exist, it falls back to creating a new
* repository with an initial version.
* repository with an initial version controlled by an initial manager.
*
* Returns the raw transaction to sign.
*
* @param {string} manager The address that has access to manage this repository.
* @param {string} manager The address that will manage the new repo if it has to be created.
* @param {string} appId The ENS name for the application repository.
* @param {string} version A valid semantic version for this version.
* @param {string} provider The name of an APM storage provider.
@@ -243,6 +246,45 @@ module.exports = (web3, options = {}) => {
* @return {Promise} A promise that resolves to a raw transaction
This conversation was marked as resolved by 0x6431346e

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Can we add the from param to the JSDoc?

*/
async publishVersion (manager, appId, version, provider, directory, contract, from) {
const {
targetContract,
name,
params
} = await this.publishVersionIntent(manager, appId, version, provider, directory, contract)

try {
const call = targetContract.methods[name](...params)

// Return transaction to sign
return {
to: targetContract.options.address,
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 conversation was marked as resolved by 0x6431346e

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)?

}
} catch (err) {
throw new Error(`Transaction would not succeed ("${err.message}")`)
}
},

/**
* Publishes a new version (`version`) of `appId` using storage provider `provider`.
This conversation was marked as resolved by 0x6431346e

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`.
*
* 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).

This comment has been minimized.

Copy link
@0x6431346e

0x6431346e Apr 26, 2019

Member

Not necessarily, I think having something like:

if ( apm.getRepo() ) {
  apm.publishVersion()
} else {
  apm.publishInitialVersion()
}

is not more difficult, but actually more expressive 👍 (you don't have to guess the magic publishVersion does).

That being said, refactoring this might take some time without adding a lot of value (especially if we want to keep backwards compatibility), so I will open an issue for it.

*
* Returns an object with the needed components to execute an aragon.js intent
*
* @param {string} manager The address that will manage the new repo if it has to be created.
* @param {string} appId The ENS name for the application repository.
* @param {string} version A valid semantic version for this version.
* @param {string} provider The name of an APM storage provider.
* @param {string} directory The directory that contains files to publish.
* @param {string} contract The new contract address for this version.
* @return {Promise} A promise that resolves to an aragon.js intent
*/
async publishVersionIntent(manager, appId, version, provider, directory, contract) {
if (!semver.valid(version)) {
throw new Error(`${version} is not a valid semantic version`)
}
@@ -260,43 +302,40 @@ module.exports = (web3, options = {}) => {
const repo = await this.getRepository(appId)
.catch(() => null)

// Default call creates a new repository and publishes the initial version
const repoRegistry = await this.getRepoRegistry(appId)
.catch(() => {
throw new Error(`Repository ${appId} does not exist and it's registry does not exist`)
})

let transactionDestination = repoRegistry.options.address
let call = repoRegistry.methods.newRepoWithVersion(
appId.split('.')[0],
manager,
formatVersion(version),
contract,
`0x${contentURI}`
)

// If the repository already exists, the call publishes a new version
// If the repo exists, create a new version in the repo
if (repo !== null) {
transactionDestination = repo.options.address
call = repo.methods.newVersion(
formatVersion(version),
contract,
`0x${contentURI}`
)
}
return {
dao: await this.getKernel(repo),
proxyAddress: repo.options.address,
name: 'newVersion',
This conversation was marked as resolved by 0x6431346e

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Perhaps we could use methodName instead?

params: [
formatVersion(version),
contract,
`0x${contentURI}`
],
targetContract: repo
}
} else {
// 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 conversation was marked as resolved by 0x6431346e

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`)
})

try {
// Return transaction to sign
return {
to: transactionDestination,
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)
dao: await this.getKernel(repoRegistry),
proxyAddress: repoRegistry.options.address,
name: 'newRepoWithVersion',
params: [
appId.split('.')[0],
manager,
formatVersion(version),
contract,
`0x${contentURI}`
],
targetContract: repoRegistry
}
} catch (err) {
throw new Error(`Transaction would not succeed ("${err.message}")`)
}
}
},
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.