[DPA-1313]: feat(deployment): add link contract deployment #15379
Conversation
| chain deployment.Chain, | ||
| ab deployment.AddressBook, | ||
| ) (*deployment.ContractDeploy[*link_token.LinkToken], error) { | ||
| linkToken, err := deployment.DeployContract[*link_token.LinkToken](lggr, chain, ab, |
There was a problem hiding this comment.
This is not a criticism of you but I really hate that we have a function which returns a result, but also has a side-effect of updating a supplied address-book. :/ That's a sort of mis-matched use of function and side effect I'm not happy with. Need to rethink it.
You're doing it right, here, I think.
There was a problem hiding this comment.
Is the link deployment needed for test environments? If not, can we not use already deployed link contracts in various chains?
There was a problem hiding this comment.
/ That's a sort of mis-matched use of function and side effect I'm not happy with. Need to rethink it.
Good to know! I was kind of following the establish pattern here as seen in the code.
Is the link deployment needed for test environments? If not, can we not use already deployed link contracts in various chains?
My understanding is that this changeset will be used for future new chains as well? And we are building a common bunch of useful changesets?
There was a problem hiding this comment.
Are we sure that this is the contract we can officially use as link contract for new chains? I was under impression this is just a test equivalent.
There was a problem hiding this comment.
If this is the intended one, please ignore me
There was a problem hiding this comment.
Good to know! I was kind of following the establish pattern here as seen in the code.
Yeah - established patterns aren't all "Good" yet. We have been evolving this under deadline pressure, so we're just now coming back to re-evaluate emerging patterns. You're not doing bad here, it's more you're echoing something that I need to rethink.
There was a problem hiding this comment.
If this is the intended one, please ignore me
@AnieeG thanks, let me double check with someone to be sure
There was a problem hiding this comment.
@AnieeG i doubled checked with Rodrigo, yes this is the one to use.
graham-chainlink
force-pushed
the
ggoh/DPA-1313/deploy-link-token
branch
from
99d23be to
dc90c89
Compare
| ) | ||
|
|
||
| // DeployLinkTokenContract deploys a link token contract to the provided chain. | ||
| func DeployLinkTokenContract( |
There was a problem hiding this comment.
nit could probably just inline this actually in the changeset, I know we do this elsewhere but we're thinking its overkill and intend to inline the deploy.go functions inside the changesets.
There was a problem hiding this comment.
ah nice, i had it right in my first iteration but then i followed the MCMS and moved the logic in to internal, let me bring the original back as i prefer that too!
Added a new changeset in shared package which deploys the link token. JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1313
graham-chainlink
dismissed stale reviews from connorwstein and cgruber
via
d7823ac
graham-chainlink
force-pushed
the
ggoh/DPA-1313/deploy-link-token
branch
from
dc90c89 to
d7823ac
Compare
Added a new changeset in shared package which deploys the link token.
JIRA: https://smartcontract-it.atlassian.net/browse/DPA-1313